fix: keep card updates inside the deck and stop PUT from inserting (#97)

Author: lesnik512

AGENTS.md

@@ -84,7 +84,7 @@ class ListDecksUseCase:
 ### Database layer
 
 - `app/models.py` — `BigIntAuditBase` from `advanced_alchemy` (auto `id`, `created_at`, `updated_at`). The module aliases `orm_registry.metadata` onto `orm.DeclarativeBase.metadata` so Alembic autogenerate sees both. New models go here.
-- `app/repositories.py` — Subclass `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`.
+- `app/repositories.py` — Subclass `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. `schemas.CardUpdates` rejects duplicate ids with a 422 before any query runs. A statement that can hit a unique constraint runs inside `wrap_sqlalchemy_exception` so that it still raises `DuplicateKeyError`.
 - `app/resources/db.py` — engine factories live here and are DI creators: modern-di resolves their parameter annotations at runtime, so `pyproject.toml` exempts `app/resources/` from ruff's `TC` rules. `create_session` passes `join_transaction_mode="create_savepoint"`. This is inert in production (the session binds to an engine) but enables the test rollback pattern below: 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.
 - `migrations/env.py` swaps the asyncpg driver for the sync `postgresql` driver and uses `app.models.METADATA` as `target_metadata`.
 

app/api/cards.py

@@ -44,7 +44,7 @@ async def create_cards(
 @ROUTER.put("/decks/{deck_id}/cards/")
 async def update_cards(
     deck_id: int,
-    data: list[schemas.CardUpdate],
+    data: schemas.CardUpdates,
     update_cards_use_case: UpdateCardsUseCase = FromDI(UpdateCardsUseCase),
 ) -> schemas.Cards:
     return schemas.Cards.from_models(await update_cards_use_case(deck_id=deck_id, data=data))

app/repositories.py

@@ -72,20 +72,24 @@ async def list_for_deck(self, deck_id: int, *, after: int | None, limit: int) ->
     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"))

app/schemas.py

@@ -1,5 +1,5 @@
 import uuid
-from typing import TYPE_CHECKING, Final, Protocol, Self
+from typing import TYPE_CHECKING, Annotated, Final, Protocol, Self
 
 import pydantic
 from pydantic import BaseModel, PositiveInt
@@ -55,6 +55,16 @@ class CardUpdate(CardBase):
     id: PositiveInt
 
 
+def _ensure_unique_ids(cards: list[CardUpdate]) -> list[CardUpdate]:
+    if len({card.id for card in cards}) != len(cards):
+        msg = "card ids must be unique"
+        raise ValueError(msg)
+    return cards
+
+
+CardUpdates = Annotated[list[CardUpdate], pydantic.AfterValidator(_ensure_unique_ids)]
+
+
 class Card(CardUpdate):
     deck_id: PositiveInt
 

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

tests/test_cards.py

@@ -182,6 +182,59 @@ async def test_update_cards_to_a_duplicate_front(client: AsyncClient) -> None:
     assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT
 
 
+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.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.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.HTTP_200_OK
+
+
+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.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.HTTP_422_UNPROCESSABLE_CONTENT
+    assert statements == []
+
+
 async def test_openapi_card_deck_id_required(client: AsyncClient) -> None:
     response = await client.get("/openapi.json")
     assert response.status_code == status.HTTP_200_OK