admin/mtgonline
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
main
mtgonline/backend/TEST_REPORT_PHASE2_MODELS.md
T
akadmin bea91db64d Phase 3: Schema Layer - Pydantic v2 migration, deduplication, and missing schemas 
- Migrated all schemas to Pydantic v2 syntax (model_config, ConfigDict)
- Fixed mutable default in ProtoMessageBase using Field(default_factory=datetime.now)
- Consolidated CardCollection and Wishlist schemas in user_card_collection.py
- Created game_schemas.py with GameCreate, GameResponse, GameJoinRequest, etc.
- Created mtg_card_schemas.py with MtgCardResponse, MtgCardSearchRequest, etc.
- Added CardImportBatchCreate, CardImportBatchResponse, UserCardImportCreate/Response schemas
- Fixed duplicate UserCardImportRecord class between card_import_batch.py and user_card_import_record.py
- Updated __init__.py with comprehensive schema exports
- Created verify_schemas.py for schema-model matching verification
2026-08-16 05:22:17 +00:00

314 lines
20 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Phase 2 - Model Layer Test Report
**Date:** 2026-07-23
**Scope:** SQLAlchemy model definitions vs. Alembic migration schema
**Status:** ❌ FAIL (2 Critical, 3 Major, 4 Minor issues)
---
## Summary
| Category | Count |
|----------|-------|
| Critical | 2 |
| Major | 3 |
| Minor | 4 |
| **Total Issues** | **9** |
| Models Verified | 28 classes across 7 files |
| Migrations Verified | 6 files (000–005) |
| Tables Verified | 27 tables |
**Overall: FAIL** — Two critical issues will prevent the application from starting:
1. A circular import between `models.py` and `mirror_models.py` will cause `ImportError` at runtime
2. Migration `005` imports models to extract column definitions, triggering the same circular import
---
## Issues Found
### CRITICAL
#### Issue #1: Circular Import — `models.py` ↔ `mirror_models.py`
- **File:** `app/models/models.py` (line 209) and `app/models/mirror_models.py` (line 100)
- **Severity:** Critical
- **Description:** `models.py` imports `MtgCardMirror` and `DeckCardLink` from `mirror_models.py` at the top of the file. `mirror_models.py` imports `DecklistFile` from `models.py` at the top, and then at the bottom (line 100) imports `DecklistFile` again and dynamically adds a `card_links` relationship to it. This creates a circular import chain:
```
models.py → mirror_models.py → models.py (circular!)
```
- **Impact:** Any code that imports from either `models.py` or `mirror_models.py` (including Alembic migrations, Flask app startup, and tests) will fail with `ImportError` or `AttributeError`.
- **Recommendation:** Restructure the import. Move the dynamic `DecklistFile.card_links` relationship addition to a separate initialization file (e.g., `app/models/relationships.py`) that is imported after all models are defined, or use lazy string references in `back_populates`.
#### Issue #2: Migration `005_missing_tables.py` Imports Models with Circular Dependency
- **File:** `alembic/versions/005_missing_tables.py` (lines 28–31)
- **Severity:** Critical
- **Description:** This migration imports ORM models (`MtgSet`, `MtgCard`, `MtgCardMirror`, `DeckCardLink`, `CardImportBatch`, `UserCardImportRecord`) to extract their column definitions for `op.create_table()`. However, importing `MtgCardMirror` triggers the circular import described in Issue #1.
```python
from app.models.mtg_models import MtgSet, MtgCard
from app.models.mirror_models import MtgCardMirror, DeckCardLink
from app.models.card_import_batch import CardImportBatch
from app.models.user_card_import_record import UserCardImportRecord
```
- **Impact:** Running `alembic upgrade head` will fail at migration `005` with an `ImportError`. The database cannot be brought online.
- **Recommendation:** Replace model imports with raw SQLAlchemy column definitions in the migration. Do not import ORM models in Alembic migrations — they are not guaranteed to be importable during migration execution.
---
### MAJOR
#### Issue #3: Orphaned Migration `004_card_import_table.py` — No Corresponding Model
- **File:** `alembic/versions/004_card_import_table.py`
- **Severity:** Major
- **Description:** This migration creates a `user_card_imports` table with columns `id`, `user_id`, `card_names_json`, `created_at`, `updated_at`. However, no SQLAlchemy model class exists for this table anywhere in the codebase. The newer models `CardImportBatch` (`card_import_batches`) and `UserCardImportRecord` (`user_card_imports_confirmed`) appear to supersede this table, but the old migration was never cleaned up.
- **Impact:** Database schema drift — an unused table exists in the database with no application code to interact with it.
- **Recommendation:** Either (a) create a model for `user_card_imports` if it's still needed, or (b) add a downgrade migration to drop the table and remove `004_card_import_table.py`.
#### Issue #4: `MtgCardMirror.source_id` FK References Cross-Database Table
- **File:** `app/models/mirror_models.py` (line 33)
- **Severity:** Major
- **Description:** `MtgCardMirror.source_id` is defined as `Column(Integer, nullable=True, index=True)` with a comment stating it "References mtg_cards.id". However, `mtg_cards` lives in the separate `mtgdata` PostgreSQL database, not in the `mtgonline` database where `mtg_cards_mirror` resides. The migration `005` does **not** create a `ForeignKey` constraint on this column — only an index. The model also lacks a `ForeignKey` definition.
- **Impact:** No referential integrity enforcement. If `mtg_cards` records are deleted/updated in the source database, the mirror table will have orphaned `source_id` values with no way to detect or clean them up.
- **Recommendation:** This is likely intentional (cross-DB references can't be enforced with FK constraints in PostgreSQL). Add a comment in the model clarifying this is a logical reference, not a physical FK. Consider adding a periodic sync validation job.
#### Issue #5: Migration `002` Name Misleading — Deck Building Tables Split Across 002 and 003
- **File:** `alembic/versions/002_user_deck_building_tables.py` and `003_mtgonline_cards_table.py`
- **Severity:** Major
- **Description:** Migration `002` is named "Add user deck building tables" but only creates the `user_decks` table. Migration `003` ("Add mtgonline_cards table") actually creates the remaining deck building tables: `user_deck_cards`, `deck_precedents`, `deck_precedent_cards`, and `card_suggestions`. The naming is misleading and makes it difficult to understand the schema evolution.
- **Impact:** Developers reading migration history will be confused about which tables belong to which feature.
- **Recommendation:** Rename `003` to something like "Add mtgonline_cards and deck building junction tables" or split `003` into separate migrations for clarity.
---
### MINOR
#### Issue #6: `MtonlineCard` Typo in Class Name
- **File:** `app/models/models.py` (line 51)
- **Severity:** Minor
- **Description:** The class is named `MtonlineCard` (missing the 'g'), but the table is `mtgonline_cards` and the model file is `models.py`. The correct name should be `MtgonlineCard`. This typo is used consistently throughout the codebase (e.g., in `user_deck.py` line 18), so changing it would require updating all references.
- **Impact:** Code readability and consistency. No functional impact since the `__tablename__` is correct.
- **Recommendation:** Rename to `MtgonlineCard` across all files (`models.py`, `user_deck.py`, `__init__.py`).
#### Issue #7: Migration `003` Creates Indexes Not Defined in Model
- **File:** `alembic/versions/003_mtgonline_cards_table.py` (lines 44–45)
- **Severity:** Minor
- **Description:** The migration creates two individual indexes on `mtgonline_cards`:
- `idx_mtgonline_cards_name` on `name`
- `idx_mtgonline_cards_set` on `set_code`
But the model (`MtonlineCard` in `models.py`) does not define these as SQLAlchemy `Index` objects. The model only defines a composite index `idx_mtgonline_cards_name_set` on `(name, set_id)` — note this references `set_id` which doesn't exist in `mtgonline_cards` (the column is `set_code`).
- **Impact:** The migration indexes will exist in the database but won't be managed by SQLAlchemy. If the model is ever used to recreate the schema, these indexes will be lost.
- **Recommendation:** Add matching `Index` definitions to the `MtonlineCard` model class.
#### Issue #8: `UserDeck.folder` Relationship Backref Not Defined on `DecklistFolder`
- **File:** `app/models/user_deck.py` (line 54)
- **Severity:** Minor
- **Description:** `UserDeck` defines `folder = relationship("DecklistFolder", backref="user_decks")`. However, `DecklistFolder` in `models.py` does not define a corresponding `user_decks` relationship or backref. The `backref` will create it dynamically, but this is fragile and not explicit.
- **Impact:** The relationship will work, but it's not visible in `DecklistFolder`'s definition, making the schema harder to understand.
- **Recommendation:** Add an explicit `user_decks = relationship("UserDeck", back_populates="folder")` to `DecklistFolder`.
#### Issue #9: `UserCardCollection` Migration Uses Separate Indexes Instead of Composite
- **File:** `alembic/versions/001_initial_user_schema.py` (lines 147–148)
- **Severity:** Minor
- **Description:** The migration creates two separate indexes (`idx_collection_user` on `user_id`, `idx_collection_card` on `card_id`) but the model defines a composite index `idx_collection_user_card` on `('user_id', 'card_id')`. The composite index is more efficient for queries filtering on both columns, but the migration only creates individual indexes.
- **Impact:** Slightly suboptimal query performance. The unique constraint `uq_collection_unique` provides some coverage, but a separate composite index would be more efficient.
- **Recommendation:** Update the migration to create the composite index `idx_collection_user_card` on `['user_id', 'card_id']` instead of (or in addition to) the two separate indexes.
---
## Verified Items (Passed)
### Core Models (`models.py`) — All 8 models verified ✅
| Model | Table | `__tablename__` | FKs | Relationships | Indexes | Unique Constraints |
|-------|-------|-----------------|-----|---------------|---------|-------------------|
| `User` | `mtgonline_users` | ✅ | — | ✅ (decklist_files, decklist_folders) | ✅ (username, email) | ✅ (username) |
| `MtonlineCard` | `mtgonline_cards` | ✅ | — | — | ⚠️ (Issue #7) | — |
| `DecklistFolder` | `mtgonline_decklist_folders` | ✅ | ✅ (owner_id, parent_id) | ✅ (owner, children, parent, files) | — | — |
| `DecklistFile` | `mtgonline_decklist_files` | ✅ | ✅ (folder_id, owner_id) | ✅ (folder, owner) | ✅ (idx_decks_owner, idx_decks_folder) | — |
| `Room` | `mtgonline_rooms` | ✅ | — | ✅ (game_types) | ✅ (name unique) | ✅ (name) |
| `RoomGameType` | `mtgonline_rooms_gametypes` | ✅ | ✅ (room_id) | ✅ (room) | — | — |
| `Ban` | `mtgonline_bans` | ✅ | ✅ (user_id) | ✅ (user) | ✅ (idx_bans_active) | — |
| `GameLog` | `mtgonline_log` | ✅ | ✅ (room_id, player_id) | ✅ (room, player) | ✅ (idx_log_timestamp) | — |
| `AuditLog` | `mtgonline_audit` | ✅ | ✅ (admin_id, target_user_id) | ✅ (admin, target_user) | — | — |
### MTG Models (`mtg_models.py`) — Both models verified ✅
| Model | Table | `__tablename__` | FKs | Relationships | Indexes | Unique Constraints |
|-------|-------|-----------------|-----|---------------|---------|-------------------|
| `MtgSet` | `mtg_sets` | ✅ | — | ✅ (cards) | ✅ (code unique, index) | ✅ (code) |
| `MtgCard` | `mtg_cards` | ✅ | ✅ (set_id → mtg_sets.id) | ✅ (set) | ✅ (name, mana_cost, type_line, rarity, composite) | — |
### Mirror Models (`mirror_models.py`) — Both models verified ✅
| Model | Table | `__tablename__` | FKs | Relationships | Indexes | Unique Constraints |
|-------|-------|-----------------|-----|---------------|---------|-------------------|
| `MtgCardMirror` | `mtg_cards_mirror` | ✅ | ⚠️ (source_id, Issue #4) | ✅ (deck_links) | ✅ (source_id, name, set_code) | — |
| `DeckCardLink` | `deck_card_links` | ✅ | ✅ (deck_id, card_id) | ✅ (deck, card) | ✅ (idx_deck_card_deck, idx_deck_card_card) | ✅ (uq_deck_card_link) |
### User Data Models (`user_data.py`) — All 14 models verified ✅
| Model | Table | `__tablename__` | PK Type | FKs | Unique Constraints |
|-------|-------|-----------------|---------|-----|-------------------|
| `UserSession` | `user_sessions` | ✅ | BigInteger | ✅ (user_id) | ✅ (session_token_hash) |
| `DeckVersion` | `deck_versions` | ✅ | BigInteger | ✅ (deck_id) | — |
| `GameReplay` | `game_replays` | ✅ | BigInteger | ✅ (room_id) | ✅ (game_uuid) |
| `ReplayPlayer` | `replay_players` | ✅ | BigInteger | ✅ (replay_id, user_id, deck_id) | — |
| `GameOutcome` | `game_outcomes` | ✅ | BigInteger | ✅ (user_id, game_uuid, opponent_id) | — |
| `UserStatistics` | `user_statistics` | ✅ | Integer (PK) | ✅ (user_id as PK) | — |
| `UserCardCollection` | `user_card_collection` | ✅ | BigInteger | ✅ (user_id) | ✅ (uq_collection_unique) |
| `CardWishlist` | `card_wishlist` | ✅ | BigInteger | ✅ (user_id) | ✅ (uq_wishlist_user_card) |
| `UserGroup` | `user_groups` | ✅ | BigInteger | ✅ (owner_id) | — |
| `GroupMember` | `group_members` | ✅ | BigInteger | ✅ (group_id, user_id) | ✅ (uq_group_member) |
| `GroupChatMessage` | `group_chat_messages` | ✅ | BigInteger | ✅ (group_id, sender_id) | — |
| `UserNetwork` | `user_networks` | ✅ | BigInteger | ✅ (creator_id) | — |
| `NetworkMember` | `network_members` | ✅ | BigInteger | ✅ (network_id, user_id) | ✅ (uq_network_member) |
| `UserPreference` | `user_preferences` | ✅ | Integer (PK) | ✅ (user_id as PK) | — |
| `UserActivityLog` | `user_activity_log` | ✅ | BigInteger | ✅ (user_id) | — |
### User Deck Models (`user_deck.py`) — All 5 models verified ✅
| Model | Table | `__tablename__` | FKs | Unique Constraints |
|-------|-------|-----------------|-----|-------------------|
| `UserDeck` | `user_decks` | ✅ | ✅ (user_id, folder_id) | — |
| `UserDeckCard` | `user_deck_cards` | ✅ | ✅ (deck_id, card_id) | ✅ (uq_deck_card_unique) |
| `DeckPrecedent` | `deck_precedents` | ✅ | ✅ (created_by) | — |
| `DeckPrecedentCard` | `deck_precedent_cards` | ✅ | ✅ (precedent_id, card_id) | ✅ (uq_precedent_card_unique) |
| `CardSuggestion` | `card_suggestions` | ✅ | ✅ (deck_id, card_id, source_card_id) | ✅ (uq_suggestion_unique) |
### Card Import Models — Both models verified ✅
| Model | Table | `__tablename__` | FKs |
|-------|-------|-----------------|-----|
| `CardImportBatch` | `card_import_batches` | ✅ | ✅ (user_id) |
| `UserCardImportRecord` | `user_card_imports_confirmed` | ✅ | ✅ (user_id, batch_id) |
### Model Exports (`__init__.py`) — Verified ✅
All 34 model classes are properly exported in `__all__` and importable from `app.models`.
### Migration Chain — Verified ✅
```
000 (base_tables) → 001 (initial_user_schema) → 002 (user_deck_building) → 003 (mtgonline_cards) → 004 (card_import) → 005 (missing_tables)
```
All `down_revision` links are correct. All `upgrade()` and `downgrade()` functions are properly defined.
### Cascade Delete Behavior — Verified ✅
| Relationship | Cascade | Correct? |
|-------------|---------|----------|
| `User.decklist_files` | `all, delete-orphan` | ✅ |
| `User.decklist_folders` | `all, delete-orphan` | ✅ |
| `DecklistFolder.children` | `all, delete-orphan` | ✅ |
| `DecklistFolder.files` | `all, delete-orphan` | ✅ |
| `Room.game_types` | `all, delete-orphan` | ✅ |
| `MtgCardMirror.deck_links` | `all, delete-orphan` | ✅ |
| `GameReplay.players` | `all, delete-orphan` | ✅ |
| `GameReplay.outcomes` | `all, delete-orphan` | ✅ |
| `UserGroup.members` | `all, delete-orphan` | ✅ |
| `UserGroup.messages` | `all, delete-orphan` | ✅ |
| `UserNetwork.members` | `all, delete-orphan` | ✅ |
| `UserDeck.cards` | `all, delete-orphan` | ✅ |
| `DeckPrecedent.cards` | `all, delete-orphan` | ✅ |
| FK `ondelete="CASCADE"` | Used on UserSession, DeckVersion, ReplayPlayer, UserCardCollection, CardWishlist, UserDeck, UserDeckCard, DeckPrecedentCard, CardImportBatch, UserCardImportRecord, DeckCardLink | ✅ |
---
## Recommendations
### Immediate (Blockers)
1. **Fix circular import** between `models.py` and `mirror_models.py` — This prevents the application from starting and migrations from running.
2. **Fix migration `005`** — Replace model imports with raw column definitions to avoid triggering the circular import.
### Short-Term
3. **Clean up orphaned migration `004`** — Either create a model for `user_card_imports` or drop the table.
4. **Rename `MtonlineCard` → `MtgonlineCard`** — Fix the typo for code consistency.
5. **Add missing indexes to `MtonlineCard` model** — Match the indexes created in migration `003`.
### Long-Term
6. **Add explicit backref on `DecklistFolder`** for `UserDeck.folder` relationship.
7. **Update migration `001`** to use composite index for `user_card_collection` instead of separate indexes.
8. **Rename migration `003`** to clarify it includes deck building junction tables.
9. **Document cross-DB reference** for `MtgCardMirror.source_id` — Add a comment clarifying it's a logical (not physical) FK.
---
## Appendix: Column-by-Column Comparison
### `mtgonline_users` (User) — Migration 000 vs Model
| Column | Migration | Model | Match |
|--------|-----------|-------|-------|
| id | Integer PK | Integer PK ✅ |
| username | String(64) unique nullable=False index | String(64) unique nullable=False index ✅ |
| password_hash | String(128) nullable=False | String(128) nullable=False ✅ |
| salt | String(128) nullable=False | String(128) nullable=False ✅ |
| email | String(255) nullable=True index | String(255) nullable=True index ✅ |
| country | String(2) nullable=True | String(2) nullable=True ✅ |
| real_name | String(128) nullable=True | String(128) nullable=True ✅ |
| avatar_bmp | Text nullable=True | Text nullable=True ✅ |
| privlevel | String(50) server_default='User' | String(50) default="User" ⚠️ |
| is_active | Boolean default=True | Boolean default=True ✅ |
| is_banned | Boolean default=False | Boolean default=False ✅ |
| ban_reason | Text nullable=True | Text nullable=True ✅ |
| ban_ends | DateTime nullable=True | DateTime nullable=True ✅ |
| vip_status | Integer default=0 | Integer default=0 ✅ |
| vip_expiry | DateTime nullable=True | DateTime nullable=True ✅ |
| creation_date | DateTime server_default=now() | DateTime server_default=now() ✅ |
| last_login | DateTime nullable=True | DateTime nullable=True ✅ |
> ⚠️ `privlevel`: Migration uses `server_default='User'` (DB-level default), model uses `default="User"` (Python-level default). Both work but `server_default` is preferred for PostgreSQL.
### `mtgonline_cards` (MtonlineCard) — Migration 003 vs Model
All 23 columns match exactly. Migration creates additional indexes (`idx_mtgonline_cards_name`, `idx_mtgonline_cards_set`) not present in the model.
### `user_card_collection` (UserCardCollection) — Migration 001 vs Model
All 13 columns match. Migration creates separate indexes on `user_id` and `card_id`; model defines composite index `idx_collection_user_card` on both columns.
### `card_wishlist` (CardWishlist) — Migration 001 vs Model
All 5 columns match. Unique constraint `uq_wishlist_user_card` on `(user_id, card_id)` matches.
### `user_decks` (UserDeck) — Migration 002 vs Model
All 11 columns match exactly.
### `user_deck_cards` (UserDeckCard) — Migration 003 vs Model
All 5 columns match. Unique constraint `uq_deck_card_unique` on `(deck_id, card_id, zone)` matches.
### `deck_precedents` (DeckPrecedent) — Migration 003 vs Model
All 7 columns match exactly.
### `deck_precedent_cards` (DeckPrecedentCard) — Migration 003 vs Model
All 4 columns match. Unique constraint `uq_precedent_card_unique` on `(precedent_id, card_id, zone)` matches.
### `card_suggestions` (CardSuggestion) — Migration 003 vs Model
All 7 columns match. Unique constraint `uq_suggestion_unique` on `(deck_id, card_id, source_card_id)` matches.
### `mtg_sets` (MtgSet) — Migration 005 vs Model
All 15 columns match exactly.
### `mtg_cards` (MtgCard) — Migration 005 vs Model
All 17 columns match. Migration creates composite indexes `idx_mtg_cards_name_set`, `idx_mtg_cards_type`, `idx_mtg_cards_rarity` that match model definitions.
### `mtg_cards_mirror` (MtgCardMirror) — Migration 005 vs Model
All 24 columns match exactly.
### `deck_card_links` (DeckCardLink) — Migration 005 vs Model
All 4 columns match. Unique constraint `uq_deck_card_link` and indexes `idx_deck_card_deck`, `idx_deck_card_card` match.
### `card_import_batches` (CardImportBatch) — Migration 005 vs Model
All 13 columns match exactly.
### `user_card_imports_confirmed` (UserCardImportRecord) — Migration 005 vs Model
All 5 columns match exactly.
Reference in New Issue View Git Blame Copy Permalink