# 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.