diff --git a/AGENTS.md b/AGENTS.md index f84b850..8cc6156 100644 --- a/AGENTS.md +++ b/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`. diff --git a/app/api/cards.py b/app/api/cards.py index 255893b..5e98e03 100644 --- a/app/api/cards.py +++ b/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)) diff --git a/app/repositories.py b/app/repositories.py index 84e774b..83d1fe5 100644 --- a/app/repositories.py +++ b/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")) diff --git a/app/schemas.py b/app/schemas.py index c15efc3..e8df93f 100644 --- a/app/schemas.py +++ b/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 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 ef9416d..1d31f07 100644 --- a/tests/test_cards.py +++ b/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