Skip to content

Bound card writes and ids, map missing decks to 404, cache the card update - #98

Merged
lesnik512 merged 1 commit into
mainfrom
fix/input-bounds-and-card-writes
Oct 17, 2026
Merged

lesnik512 merged 1 commit into
mainfrom
fix/input-bounds-and-card-writes

Conversation

@lesnik512

Copy link
Copy Markdown
Member

Fixes the confirmed 500s and two performance gaps that came up in a review of the template. Each fix has a test that failed on main first.

Bugs

  • Too many cards. PUT /decks/{id}/cards/ sent four bind parameters per card, and asyncpg rejects more than 32767, so 8192 cards gave a 500. That fits well under the 1 MB body limit. POST and PUT now accept at most MAX_CARDS_PER_REQUEST (1000) cards and return 422 before any query runs.
  • Ids above bigint. Path ids, after and CardUpdate.id had no upper bound, so 2**63 reached asyncpg and failed with OverflowError as a 500. They are now bounded by schemas.MAX_ID and return 422. Path ids also require gt=0 now, so /decks/0/ returns 422 instead of 404. Two existing tests that used id 0 for "not found" now use 999.
  • POST cards to a missing deck. The foreign key violation surfaced as advanced-alchemy's ForeignKeyError, which had no handler, so it was a 500. It now maps to 404, the same as a missing deck everywhere else.
  • LOG_LEVEL. LogLevels(settings.log_level) accepted only lowercase values, so LOG_LEVEL=INFO stopped the server at startup. The setting only reached Granian, so LOG_LEVEL=warning left app logs at INFO. The field is now a case-insensitive LogLevels and also sets logging_log_level through granian's log_levels_map.
  • Deck detail card order. The cards relationship had no order_by, so after an update the cards came back in heap order. The new test reproduced this on main. It now uses order_by="Card.id", the same order as the list endpoints.

Performance

  • PUT cards is now cacheable. SQLAlchemy never caches a multi-row VALUES. In cache_key.py, visit_dml_multi_values sets NO_CACHE with the comment "multivalues are simply not cacheable right now". So every PUT compiled from scratch. The SQL text also changed with the number of cards, so each new count cost asyncpg a Parse round trip and a slot in its 100-entry statement cache. The statement now reads FROM unnest($ids::BIGINT[], $fronts::VARCHAR[], ...) AS updates(id, front, back, hint). That keeps one SQL text for any count and four parameters in total. A test checks that PUTs with one card and with two cards send identical SQL.
  • Index for card pagination. list_for_deck runs WHERE deck_id = ? AND id > ? ORDER BY id LIMIT n. The only indexes were the primary key and the unique (deck_id, front), so Postgres had to sort a whole deck or walk the primary key and filter. This adds ix_cards_deck_id_id and a migration. The pytest-alembic suite checks that the model and the migration match.

Small cleanups

  • update_cards no longer sets updated_at=sa.func.now() by hand. The column's onupdate already fires for a Core UPDATE, and deck updates rely on it, so both tables now take updated_at from the same clock. test_update_cards_moves_updated_at still passes.
  • The DeckWithCards docstring said selectinload, but the code uses joinedload.
  • pyproject.toml exempts app/api/*.py from ruff's TC rules, because FastAPI reads route annotations at runtime and ruff would otherwise move the shared PathId import under TYPE_CHECKING.

…ard update

Cap card lists at MAX_CARDS_PER_REQUEST and ids at bigint so oversized input is a 422 instead of a 500. Map ForeignKeyError to 404. Make LOG_LEVEL case-insensitive and pass it to app logging. Order deck detail cards by id. Send PUT cards through unnest so the statement is cached and prepared once. Add a (deck_id, id) index for card pagination.
@lesnik512
lesnik512 merged commit ee237fd into main Oct 17, 2026
3 checks passed
@lesnik512
lesnik512 deleted the fix/input-bounds-and-card-writes branch October 17, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant