admin/mtgonline
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Phase 3: Schema Layer - Pydantic v2 migration, deduplication, and missing schemas

Browse Source 
- 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
This commit is contained in:
akadmin
2026-08-16 05:22:17 +00:00
parent 5f324cf8a9
commit bea91db64d
20 changed files with 2733 additions and 342 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/test_reports/phase2_model_layer_test_report.md
+385
View File
@@ -0,0 +1,385 @@
# Phase 2: Model Layer Test Report
**Date:** 2026-07-23
**Project:** MTG Online Backend API
**Location:** `/home/wall-o/projects/mtgonline/backend/app/models/`
**Database:** PostgreSQL (Alembic migrations 000–005)
**Framework:** FastAPI + SQLAlchemy async
---
## Executive Summary
| File | Status | Models Tested | Issues Found |
|------|--------|---------------|--------------|
| `user_data.py` | ⚠️ PASS (with issues) | 15 | 5 |
| `user_deck.py` | ⚠️ PASS (with issues) | 5 | 3 |
| `user_card_import.py` | ✅ PASS | 1 | 0 |
| `user_card_import_record.py` | ✅ PASS | 1 | 0 |
| `card_import_batch.py` | ✅ PASS | 1 | 0 |
| `mtg_models.py` | ✅ PASS | 2 | 0 |
| `mirror_models.py` | ✅ PASS | 2 | 0 |
| `models.py` | ✅ PASS | 8 | 0 |
| `__init__.py` | ⚠️ PASS (with issues) | — | 2 |
| **TOTAL** | **⚠️ 10 files** | **35 models** | **10 issues** |
---
## 1. User Data Models (`app/models/user_data.py`) — ⚠️ PASS (5 issues)
**Models tested:** 15 classes
**Migration reference:** 001 (`001_initial_user_schema.py`)
### Models Verified (all columns match migration):
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `UserSession` | `user_sessions` | `BigInteger` ✓ | `mtgonline_users.id` (CASCADE) ✓ | ✓ |
| `DeckVersion` | `deck_versions` | `BigInteger` ✓ | `mtgonline_decklist_files.id` (CASCADE) ✓ | ✓ |
| `GameReplay` | `game_replays` | `BigInteger` ✓ | `mtgonline_rooms.id` ✓ | ✓ |
| `ReplayPlayer` | `replay_players` | `BigInteger` ✓ | `game_replays.id` (CASCADE), `mtgonline_users.id`, `mtgonline_decklist_files.id` ✓ | ✓ |
| `GameOutcome` | `game_outcomes` | `BigInteger` ✓ | `mtgonline_users.id`, `game_replays.game_uuid` ✓ | ✓ |
| `UserStatistics` | `user_statistics` | `user_id` (composite PK) ✓ | `mtgonline_users.id` ✓ | ✓ |
| `UserCardCollection` | `user_card_collection` | `BigInteger` ✓ | `mtgonline_users.id` (CASCADE) ✓ | ✓ |
| `CardWishlist` | `card_wishlist` | `BigInteger` ✓ | `mtgonline_users.id` (CASCADE) ✓ | ✓ |
| `UserGroup` | `user_groups` | `BigInteger` ✓ | `mtgonline_users.id` ✓ | ✓ |
| `GroupMember` | `group_members` | `BigInteger` ✓ | `user_groups.id` (CASCADE), `mtgonline_users.id` ✓ | ✓ |
| `GroupChatMessage` | `group_chat_messages` | `BigInteger` ✓ | `user_groups.id` (CASCADE), `mtgonline_users.id` ✓ | ✓ |
| `UserNetwork` | `user_networks` | `BigInteger` ✓ | `mtgonline_users.id` ✓ | ✓ |
| `NetworkMember` | `network_members` | `BigInteger` ✓ | `user_networks.id` (CASCADE), `mtgonline_users.id` ✓ | ✓ |
| `UserPreference` | `user_preferences` | `user_id` (composite PK) ✓ | `mtgonline_users.id` ✓ | ✓ |
| `UserActivityLog` | `user_activity_log` | `BigInteger` ✓ | `mtgonline_users.id` ✓ | ✓ |
### Issues Found:
#### ISSUE-1: Missing `backref` on `User` model for 5 relationships
**Severity:** Medium
**Files:** `user_data.py`, `models.py`
The following models define `backref` on their relationship to `User`, but `User` (in `models.py`) does not define the corresponding reverse relationship:
| Model | backref defined | Missing on `User` |
|-------|----------------|-------------------|
| `UserSession` | `backref="sessions"` | `User.sessions` ✗ |
| `UserStatistics` | `backref="statistics"` | `User.statistics` ✗ |
| `UserCardCollection` | `backref="card_collection"` | `User.card_collection` ✗ |
| `CardWishlist` | `backref="wishlist"` | `User.wishlist` ✗ |
| `UserActivityLog` | `backref="activity_logs"` | `User.activity_logs` ✗ |
**Impact:** `user.sessions`, `user.statistics`, `user.card_collection`, `user.wishlist`, and `user.activity_logs` will raise `AttributeError` at runtime.
**Fix:** Add corresponding relationships to `User` in `models.py`:
```python
sessions = relationship("UserSession", back_populates="user")
statistics = relationship("UserStatistics", back_populates="user")
card_collection = relationship("UserCardCollection", back_populates="user")
wishlist = relationship("CardWishlist", back_populates="user")
activity_logs = relationship("UserActivityLog", back_populates="user")
```
#### ISSUE-2: `UserGroup.owner` uses `backref` but `User` lacks `groups` relationship
**Severity:** Low
**Files:** `user_data.py`, `models.py`
`UserGroup.owner` defines `relationship("User", foreign_keys=[owner_id])` with no `backref` or `back_populates`. This is intentional (no reverse nav), but `User` also lacks an explicit `groups` relationship. If code expects `user.groups`, it will fail.
**Recommendation:** Add `groups = relationship("UserGroup", foreign_keys="[UserGroup.owner_id]", back_populates="owner")` to `User` if bidirectional navigation is needed.
#### ISSUE-3: `UserGroup.members` / `UserGroup.messages` cascade delete
**Severity:** Informational
**Files:** `user_data.py`
`UserGroup.members` and `UserGroup.messages` both use `cascade="all, delete-orphan"`. This means deleting a `UserGroup` will also delete all `GroupMember` and `GroupChatMessage` rows. This is consistent with the migration (FKs use `ondelete="CASCADE"`) and is likely intentional.
**Status:** No action needed — behavior is correct.
#### ISSUE-4: `UserCardCollection.card_id` has no FK constraint
**Severity:** Low
**Files:** `user_data.py`, migration 001
`card_id` is `Column(Integer, nullable=False, index=True)` with no `ForeignKey()` constraint. The migration confirms this: `sa.Column('card_id', sa.Integer(), nullable=False)`. This means the database will not enforce referential integrity for card references.
**Impact:** Orphaned `card_id` values are possible. If cards are looked up by `card_id`, invalid values will silently return no results.
**Recommendation:** Add `ForeignKey("mtg_cards.id")` if card references should be enforced, or document that `card_id` is a free-form identifier.
#### ISSUE-5: `UserGroup.owner` missing `backref`
**Severity:** Low
**Files:** `user_data.py`, `models.py`
`UserGroup.owner = relationship("User", foreign_keys=[owner_id])` — no `backref` defined. `User` has no `groups` relationship. If code expects `user.groups` to return the groups the user owns, it will fail.
**Recommendation:** Add `backref="groups"` or add explicit `groups` relationship to `User`.
---
## 2. User Deck Models (`app/models/user_deck.py`) — ⚠️ PASS (3 issues)
**Models tested:** 5 classes
**Migration reference:** 002 (`002_user_deck_building_tables.py`), 003 (`003_mtgonline_cards_table.py`)
### Models Verified:
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `UserDeck` | `user_decks` | `BigInteger` autoincrement ✓ | `mtgonline_users.id` (CASCADE), `mtgonline_decklist_folders.id` ✓ | ✓ |
| `UserDeckCard` | `user_deck_cards` | `BigInteger` autoincrement ✓ | `user_decks.id` (CASCADE), `mtgonline_cards.id` ✓ | ✓ |
| `DeckPrecedent` | `deck_precedents` | `BigInteger` autoincrement ✓ | `mtgonline_users.id` ✓ | ✓ |
| `DeckPrecedentCard` | `deck_precedent_cards` | `BigInteger` autoincrement ✓ | `deck_precedents.id` (CASCADE), `mtgonline_cards.id` ✓ | ✓ |
| `CardSuggestion` | `card_suggestions` | `BigInteger` autoincrement ✓ | `user_decks.id` (CASCADE), `mtgonline_cards.id`, `mtgonline_cards.id` (source) ✓ | ✓ |
### Issues Found:
#### ISSUE-6: Unused import of `MtgonlineCard` in `user_deck.py`
**Severity:** Low
**Files:** `user_deck.py`
Line 13: `from app.models.models import MtgonlineCard` — this import is **not used** in the `UserDeck` class. It is only used in `UserDeckCard`.
**Fix:** Remove the import from the top of `user_deck.py` and keep it only where needed (or move it to module-level if `UserDeckCard` needs it at import time).
#### ISSUE-7: `UserDeck.folder` missing `backref` on `DecklistFolder`
**Severity:** Low
**Files:** `user_deck.py`, `models.py`
`UserDeck.folder = relationship("DecklistFolder", backref="user_decks")` — but `DecklistFolder` in `models.py` does not define a `user_decks` relationship. Only `owner`, `children`, `parent`, and `files` are defined.
**Impact:** `folder.user_decks` will raise `AttributeError`.
**Fix:** Add `user_decks = relationship("UserDeck", back_populates="folder")` to `DecklistFolder` in `models.py`.
#### ISSUE-8: `CardSuggestion` missing `card` and `source_card` relationships
**Severity:** Medium
**Files:** `user_deck.py`
`CardSuggestion` has foreign keys `card_id` (→ `mtgonline_cards.id`) and `source_card_id` (→ `mtgonline_cards.id`), but defines **no relationships** for either:
- No `card = relationship("MtgonlineCard", ...)` for `card_id`
- No `source_card = relationship("MtgonlineCard", ...)` for `source_card_id`
**Impact:** Cannot navigate from a suggestion to the suggested card or the source card that triggered it.
**Fix:**
```python
card = relationship("MtgonlineCard", foreign_keys=[card_id], backref="suggested_in")
source_card = relationship("MtgonlineCard", foreign_keys=[source_card_id], backref="source_for")
```
---
## 3. Card Import Models — ✅ PASS (0 issues)
### `user_card_import.py` — ✅ PASS
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `UserCardImport` | `user_card_imports` | `Integer` autoincrement ✓ | `mtgonline_users.id` (CASCADE), unique ✓ | ✓ |
- Column `card_names_json` matches migration `Text()` ✓
- Unique constraint on `user_id` matches migration `UniqueConstraint('user_id')` ✓
- Relationship `user = relationship("User", backref="card_imports")` — `User` lacks `card_imports` (same pattern as ISSUE-1)
### `user_card_import_record.py` — ✅ PASS
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `UserCardImportRecord` | `user_card_imports_confirmed` | `Integer` autoincrement ✓ | `mtgonline_users.id` (CASCADE), `card_import_batches.id` (CASCADE) ✓ | ✓ |
- All columns match migration 005 ✓
- Relationships bidirectional (`back_populates`) ✓
### `card_import_batch.py` — ✅ PASS
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `CardImportBatch` | `card_import_batches` | `Integer` autoincrement ✓ | `mtgonline_users.id` (CASCADE) ✓ | ✓ |
- All columns match migration 005 ✓
- `match_results` is `JSON` type ✓
- Relationship `user` with `backref="import_batches"` — `User` lacks `import_batches` (same pattern as ISSUE-1)
---
## 4. Game Models (`app/models/user_data.py` — `GameReplay`, `ReplayPlayer`, `GameOutcome`) — Covered in Section 1
All three models verified against migration 001. No additional issues beyond those in Section 1.
---
## 5. MTG Card Models — ✅ PASS (0 issues)
### `mtg_models.py` — ✅ PASS
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `MtgSet` | `mtg_sets` | `Integer` ✓ | None ✓ | ✓ |
| `MtgCard` | `mtg_cards` | `Integer` ✓ | `mtg_sets.id` ✓ | ✓ |
- All columns match migration 005 ✓
- Relationships bidirectional (`back_populates`) ✓
- Indexes defined at module level (`idx_mtg_cards_name_set`, `idx_mtg_cards_type`, `idx_mtg_cards_rarity`) ✓
### `mirror_models.py` — ✅ PASS
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `MtgCardMirror` | `mtg_cards_mirror` | `Integer` ✓ | None ✓ | ✓ |
| `DeckCardLink` | `deck_card_links` | `Integer` ✓ | `mtgonline_decklist_files.id` (CASCADE), `mtg_cards_mirror.id` ✓ | ✓ |
- All columns match migration 005 ✓
- Relationships bidirectional (`back_populates`) ✓
- `DeckCardLink` unique constraint on `(deck_id, card_id, zone)` ✓
- Late relationship addition to `DecklistFile.card_links` works correctly (import order is valid) ✓
---
## 6. Base Models (`app/models/models.py`) — ✅ PASS (0 issues)
**Models tested:** 8 classes
| Model | Table | Primary Key | FK References | Status |
|-------|-------|-------------|---------------|--------|
| `User` | `mtgonline_users` | `Integer` ✓ | None ✓ | ✓ |
| `MtgonlineCard` | `mtgonline_cards` | `Integer` ✓ | None ✓ | ✓ |
| `DecklistFolder` | `mtgonline_decklist_folders` | `Integer` ✓ | `mtgonline_users.id`, self-ref ✓ | ✓ |
| `DecklistFile` | `mtgonline_decklist_files` | `Integer` ✓ | `mtgonline_decklist_folders.id`, `mtgonline_users.id` ✓ | ✓ |
| `Room` | `mtgonline_rooms` | `Integer` ✓ | None ✓ | ✓ |
| `RoomGameType` | `mtgonline_rooms_gametypes` | `Integer` ✓ | `mtgonline_rooms.id` ✓ | ✓ |
| `Ban` | `mtgonline_bans` | `Integer` ✓ | `mtgonline_users.id` ✓ | ✓ |
| `GameLog` | `mtgonline_log` | `Integer` ✓ | `mtgonline_rooms.id`, `mtgonline_users.id` ✓ | ✓ |
| `AuditLog` | `mtgonline_audit` | `Integer` ✓ | `mtgonline_users.id` (admin), `mtgonline_users.id` (target) ✓ | ✓ |
- All columns match migrations 000 ✓
- Relationships properly defined with `back_populates` or `backref` ✓
- Self-referential relationship on `DecklistFolder` (parent/children) ✓
- Dual FK to `User` on `AuditLog` with `foreign_keys` ✓
- Module-level indexes defined ✓
---
## 7. Model Imports (`app/models/__init__.py`) — ⚠️ PASS (2 issues)
### Exports Verified:
All 35 models are properly imported and listed in `__all__`:
- `User`, `DecklistFile`, `DecklistFolder`, `Room`, `RoomGameType`, `Ban`, `GameLog`, `AuditLog` ✓
- `MtgSet`, `MtgCard` ✓
- `MtgCardMirror`, `DeckCardLink` ✓
- `UserSession`, `DeckVersion`, `GameReplay`, `ReplayPlayer`, `GameOutcome`, `UserStatistics`, `UserCardCollection`, `CardWishlist`, `UserGroup`, `GroupMember`, `GroupChatMessage`, `UserNetwork`, `NetworkMember`, `UserPreference`, `UserActivityLog` ✓
- `UserDeck`, `UserDeckCard`, `DeckPrecedent`, `DeckPrecedentCard`, `CardSuggestion` ✓
- `CardImportBatch`, `UserCardImportRecord` ✓
### Issues Found:
#### ISSUE-9: `MtgonlineCard` not exported from `__init__.py`
**Severity:** Low
**Files:** `__init__.py`
`MtgonlineCard` is defined in `models.py` and used by `UserDeckCard` (via direct import `from app.models.models import MtgonlineCard`), but it is **not** included in `__init__.py`'s imports or `__all__`.
**Impact:** Code that tries `from app.models import MtgonlineCard` will fail. Current code works because `UserDeckCard` imports directly from `app.models.models`.
**Recommendation:** Add `MtgonlineCard` to `__init__.py` imports and `__all__` for consistency.
#### ISSUE-10: `UserCardImport` not exported from `__init__.py`
**Severity:** Informational
**Files:** `__init__.py`
`UserCardImport` is defined in `user_card_import.py` but is **not** imported or listed in `__init__.py`.
**Impact:** Cannot access via `from app.models import UserCardImport`. Must use `from app.models.user_card_import import UserCardImport`.
**Recommendation:** Add `UserCardImport` to `__init__.py` imports and `__all__` for consistency with other models.
---
## 8. Cross-File Consistency Checks
### Foreign Key Reference Validation
All foreign keys reference tables that exist in the migration chain:
| FK Target Table | Defined In | Status |
|-----------------|-----------|--------|
| `mtgonline_users` | Migration 000 | ✓ |
| `mtgonline_decklist_files` | Migration 000 | ✓ |
| `mtgonline_decklist_folders` | Migration 000 | ✓ |
| `mtgonline_rooms` | Migration 000 | ✓ |
| `game_replays` | Migration 001 | ✓ |
| `mtgonline_cards` | Migration 003 | ✓ |
| `mtg_sets` | Migration 005 | ✓ |
| `mtg_cards_mirror` | Migration 005 | ✓ |
| `card_import_batches` | Migration 005 | ✓ |
| `user_decks` | Migration 002 | ✓ |
| `deck_precedents` | Migration 003 | ✓ |
### Relationship Bidirectionality Audit
| Relationship | Forward | Reverse | Status |
|-------------|---------|---------|--------|
| `UserSession.user` ↔ `User` | `backref="sessions"` | Missing on `User` | ⚠️ ISSUE-1 |
| `UserStatistics.user` ↔ `User` | `backref="statistics"` | Missing on `User` | ⚠️ ISSUE-1 |
| `UserCardCollection.user` ↔ `User` | `backref="card_collection"` | Missing on `User` | ⚠️ ISSUE-1 |
| `CardWishlist.user` ↔ `User` | `backref="wishlist"` | Missing on `User` | ⚠️ ISSUE-1 |
| `UserActivityLog.user` ↔ `User` | `backref="activity_logs"` | Missing on `User` | ⚠️ ISSUE-1 |
| `CardImportBatch.user` ↔ `User` | `backref="import_batches"` | Missing on `User` | ⚠️ ISSUE-1 |
| `GameReplay.players` ↔ `ReplayPlayer` | `back_populates` | `back_populates` | ✓ |
| `GameReplay.outcomes` ↔ `GameOutcome` | `back_populates` | `back_populates` | ✓ |
| `UserGroup.members` ↔ `GroupMember` | `back_populates` | `back_populates` | ✓ |
| `UserGroup.messages` ↔ `GroupChatMessage` | `back_populates` | `back_populates` | ✓ |
| `UserNetwork.members` ↔ `NetworkMember` | `back_populates` | `back_populates` | ✓ |
| `MtgSet.cards` ↔ `MtgCard` | `back_populates` | `back_populates` | ✓ |
| `MtgCardMirror.deck_links` ↔ `DeckCardLink` | `back_populates` | `back_populates` | ✓ |
| `DecklistFile.card_links` ↔ `DeckCardLink` | Late-added | `back_populates` | ✓ |
| `UserDeck.cards` ↔ `UserDeckCard` | `back_populates` | `back_populates` | ✓ |
| `DeckPrecedent.cards` ↔ `DeckPrecedentCard` | `back_populates` | `back_populates` | ✓ |
---
## Summary of All Issues
| ID | Severity | File(s) | Description |
|----|----------|---------|-------------|
| 1 | Medium | `user_data.py`, `models.py` | 6 models use `backref` to `User` but `User` lacks corresponding relationships (`sessions`, `statistics`, `card_collection`, `wishlist`, `activity_logs`, `import_batches`) |
| 2 | Low | `user_data.py`, `models.py` | `UserGroup.owner` has no `backref`; `User` lacks `groups` relationship |
| 3 | Info | `user_data.py` | `UserGroup.members`/`messages` cascade delete — correct but verify intentional |
| 4 | Low | `user_data.py` | `UserCardCollection.card_id` has no FK constraint to any cards table |
| 5 | Low | `user_data.py`, `models.py` | `UserGroup.owner` missing `backref` for bidirectional nav |
| 6 | Low | `user_deck.py` | Unused import `MtgonlineCard` at top of file |
| 7 | Low | `user_deck.py`, `models.py` | `UserDeck.folder` uses `backref="user_decks"` but `DecklistFolder` lacks it |
| 8 | Medium | `user_deck.py` | `CardSuggestion` missing `card` and `source_card` relationships for its FK columns |
| 9 | Low | `__init__.py` | `MtgonlineCard` not exported from models package |
| 10 | Info | `__init__.py` | `UserCardImport` not exported from models package |
---
## Recommendations
### Priority 1 (Fix Before Production)
1. **ISSUE-1**: Add missing `backref` relationships to `User` in `models.py` — 6 runtime `AttributeError` risks
2. **ISSUE-8**: Add `card` and `source_card` relationships to `CardSuggestion` — missing navigation for FK columns
### Priority 2 (Fix Soon)
3. **ISSUE-7**: Add `user_decks` relationship to `DecklistFolder` in `models.py`
4. **ISSUE-9**: Export `MtgonlineCard` from `__init__.py`
5. **ISSUE-10**: Export `UserCardImport` from `__init__.py`
### Priority 3 (Consider)
6. **ISSUE-2/5**: Decide whether `User.groups` navigation is needed; add if so
7. **ISSUE-4**: Add FK constraint to `UserCardCollection.card_id` or document as free-form
8. **ISSUE-6**: Remove unused `MtgonlineCard` import from `user_deck.py`
---
## Test Methodology
1. Read all 8 model files and 6 migration files
2. Compared every column definition (type, nullable, default, index, FK, constraint) between models and migrations
3. Verified all `__tablename__` values match migration table names
4. Checked all `relationship()` calls for proper `back_populates` / `backref` pairing
5. Validated foreign key target tables exist in the migration chain
6. Verified `__init__.py` exports all model classes
7. Checked for unused imports and missing imports
---
*Report generated: 2026-07-23*