cal/paper-dynasty-database
1
0
Fork 0
Code Issues 7 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

fix: refactor Roster from 26 FK columns to RosterSlot junction table (#29) #58

Merged
cal merged 1 commits from ai/paper-dynasty-database#29 into next-release 2026-03-07 03:23:42 +00:00
Conversation 1 Commits 1 Files Changed 2 +84 -35
cal commented 2026-03-04 15:33:31 +00:00
Owner
Copy Link

Summary

  • Removes the 26 individual card_1..card_26 FK columns from the Roster Peewee model
  • Adds a RosterSlot junction table model with (roster, slot, card) fields and a unique index on (roster, slot)
  • Activates the get_cards() helper on Roster — now returns cards ordered by slot via a proper JOIN
  • Registers RosterSlot in the create_tables block for SQLite dev environments
  • Adds migrations/migrate_roster_junction_table.py to backfill existing data from the legacy card_N_id columns

Files Changed

  • app/db_engine.py — model changes
  • migrations/migrate_roster_junction_table.py — data migration script (new)

Notes

The legacy card_1..card_26 columns remain in the actual database schema (both SQLite dev and PostgreSQL prod) until the migration script is run and an operator drops them. The ORM model no longer references them, so they're silently ignored by Peewee.

To migrate existing rosters run:

python migrations/migrate_roster_junction_table.py

Test plan

  • Review RosterSlot model definition and unique index
  • Run migration script against dev SQLite DB and verify row count in rosterslot
  • Verify roster.get_cards() returns cards in slot order
  • Deploy to staging and confirm no startup errors

Closes #29

## Summary - Removes the 26 individual `card_1`..`card_26` FK columns from the `Roster` Peewee model - Adds a `RosterSlot` junction table model with `(roster, slot, card)` fields and a unique index on `(roster, slot)` - Activates the `get_cards()` helper on `Roster` — now returns cards ordered by slot via a proper JOIN - Registers `RosterSlot` in the `create_tables` block for SQLite dev environments - Adds `migrations/migrate_roster_junction_table.py` to backfill existing data from the legacy `card_N_id` columns ## Files Changed - `app/db_engine.py` — model changes - `migrations/migrate_roster_junction_table.py` — data migration script (new) ## Notes The legacy `card_1..card_26` columns remain in the actual database schema (both SQLite dev and PostgreSQL prod) until the migration script is run and an operator drops them. The ORM model no longer references them, so they're silently ignored by Peewee. To migrate existing rosters run: ``` python migrations/migrate_roster_junction_table.py ``` ## Test plan - [ ] Review `RosterSlot` model definition and unique index - [ ] Run migration script against dev SQLite DB and verify row count in `rosterslot` - [ ] Verify `roster.get_cards()` returns cards in slot order - [ ] Deploy to staging and confirm no startup errors Closes #29
cal added 1 commit 2026-03-04 15:33:32 +00:00 
- Remove card_1..card_26 FK columns from Roster ORM model
- Add RosterSlot model with (roster, slot, card) and a unique index on (roster, slot)
- Activate get_cards() helper on Roster using the new junction table
- Register RosterSlot in create_tables for SQLite dev environments
- Add migrations/migrate_roster_junction_table.py to backfill existing data

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
ai-reviewing
 label 2026-03-05 03:47:43 +00:00
cal reviewed 2026-03-05 03:50:10 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/db_engine.py (modified)
  • migrations/migrate_roster_junction_table.py (added)

Findings

Correctness

  • Roster.get_cards() correctly joins Card → RosterSlot via Peewee's FK backref (RosterSlot.card = ForeignKeyField(Card, backref="roster_slots")). Generated SQL: SELECT card.* FROM card INNER JOIN rosterslot ON (rosterslot.card_id = card.id) WHERE rosterslot.roster_id = ? ORDER BY rosterslot.slot — correct and efficient.
  • RosterSlot is defined after Roster. The forward reference in get_cards() is safe — method bodies are evaluated lazily; by the time any request calls get_cards(), the module is fully loaded and RosterSlot is in scope. ✓
  • Migration correctly looks up card_{n}_id (Peewee appends _id to FK column names). ✓
  • already_migrated guard is per-roster — idempotent and correct for a one-shot migration. ✓
  • RosterSlot is inserted after Roster in the create_tables list, satisfying FK dependency ordering for fresh SQLite envs. ✓
  • Impact assessment: Grepped all routers — no code directly accesses roster.card_1..card_26 attributes, and get_cards() has no existing callers. Zero breaking changes. ✓

Security

  • No issues. Migration uses Peewee ORM parameterized inserts, not raw SQL string interpolation. ✓

Style & Conventions

  • Follows existing Peewee model patterns in db_engine.py. ✓
  • indexes = (((\"roster\", \"slot\"), True),) is correct Peewee composite unique index syntax. ✓

Suggestions

  • The migration's summary silently ignores rosters where all 26 slots are NULL — they're not counted in migrated or skipped. Functionally correct; consider a third empty counter for operational clarity if needed (not a blocker).
  • No PostgreSQL-specific SQL file, unlike the scout PR pattern. The Python script handles both engines via db.create_tables() — sufficient, but ensure the script is run before the new image goes live in prod.

Verdict: APPROVED

Clean design. Junction table refactor is correct, migration is idempotent, no existing callers are broken, and get_cards() is logically sound. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified) - `migrations/migrate_roster_junction_table.py` (added) ### Findings #### Correctness - `Roster.get_cards()` correctly joins `Card → RosterSlot` via Peewee's FK backref (`RosterSlot.card = ForeignKeyField(Card, backref="roster_slots")`). Generated SQL: `SELECT card.* FROM card INNER JOIN rosterslot ON (rosterslot.card_id = card.id) WHERE rosterslot.roster_id = ? ORDER BY rosterslot.slot` — correct and efficient. - `RosterSlot` is defined after `Roster`. The forward reference in `get_cards()` is safe — method bodies are evaluated lazily; by the time any request calls `get_cards()`, the module is fully loaded and `RosterSlot` is in scope. ✓ - Migration correctly looks up `card_{n}_id` (Peewee appends `_id` to FK column names). ✓ - `already_migrated` guard is per-roster — idempotent and correct for a one-shot migration. ✓ - `RosterSlot` is inserted after `Roster` in the `create_tables` list, satisfying FK dependency ordering for fresh SQLite envs. ✓ - **Impact assessment**: Grepped all routers — no code directly accesses `roster.card_1..card_26` attributes, and `get_cards()` has no existing callers. Zero breaking changes. ✓ #### Security - No issues. Migration uses Peewee ORM parameterized inserts, not raw SQL string interpolation. ✓ #### Style & Conventions - Follows existing Peewee model patterns in `db_engine.py`. ✓ - `indexes = (((\"roster\", \"slot\"), True),)` is correct Peewee composite unique index syntax. ✓ #### Suggestions - The migration's summary silently ignores rosters where all 26 slots are NULL — they're not counted in `migrated` or `skipped`. Functionally correct; consider a third `empty` counter for operational clarity if needed (not a blocker). - No PostgreSQL-specific SQL file, unlike the scout PR pattern. The Python script handles both engines via `db.create_tables()` — sufficient, but ensure the script is run before the new image goes live in prod. ### Verdict: APPROVED Clean design. Junction table refactor is correct, migration is idempotent, no existing callers are broken, and `get_cards()` is logically sound. Ready to merge. --- *Automated review by Claude PR Reviewer*
cal added the
ai-reviewed
 label 2026-03-05 03:50:25 +00:00
cal force-pushed ai/paper-dynasty-database#29 from 5003b459e6 to 44b6222ad5 2026-03-05 21:34:46 +00:00 Compare
cal commented 2026-03-07 03:23:34 +00:00
Author
Owner
Copy Link

TODO: Confirm migration is completed on both dev and prod prior to release.

TODO: Confirm migration is completed on both dev and prod prior to release.
cal merged commit 7295e77c96 into next-release 2026-03-07 03:23:42 +00:00
cal deleted branch ai/paper-dynasty-database#29 2026-03-07 03:23:43 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working

   
bug

   
enhancement

   
evolution

Card Evolution system

  
performance

   
phase-0

Render Pipeline Optimization

  
phase-1a

Schema & Data Foundation

  
phase-1b

API & Formula Engine

  
phase-1c

Bot Integration

  
phase-1d

Deployment

  
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
tech-debt
todo
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#58
No description provided.
Delete Branch "ai/paper-dynasty-database#29"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?