Add test reports from Phases 3-5: schema, router, and service layer analysis
This commit is contained in:
@@ -0,0 +1,226 @@
|
||||
# Phase 3: Schema Layer Test Report
|
||||
**Date:** 2026-06-25
|
||||
**Scope:** All Pydantic schemas in `/home/wall-o/projects/mtgonline/backend/app/schemas/`
|
||||
**Models Compared Against:** All ORM models in `/home/wall-o/projects/mtgonline/backend/app/models/`
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Schema File | Status | Critical | Warning | Info |
|
||||
|---|---|---|---|---|
|
||||
| `schemas.py` | ⚠️ FAIL | 1 | 2 | 1 |
|
||||
| `user_data_schemas.py` | ⚠️ FAIL | 1 | 2 | 3 |
|
||||
| `user_deck_schemas.py` | ⚠️ FAIL | 1 | 3 | 2 |
|
||||
| `card_import_schemas.py` | ⚠️ FAIL | 0 | 2 | 2 |
|
||||
| `user_card_collection.py` | ⚠️ FAIL | 0 | 1 | 2 |
|
||||
| `card_search_schemas.py` | ⚠️ FAIL | 1 | 3 | 2 |
|
||||
| `__init__.py` | ⚠️ FAIL | 1 | 0 | 0 |
|
||||
| **TOTAL** | **6 FAIL** | **5** | **13** | **12** |
|
||||
|
||||
---
|
||||
|
||||
## 1. `schemas.py` — ⚠️ FAIL
|
||||
|
||||
**Models covered:** User, DecklistFile, DecklistFolder, Room, Ban, MtgCardMirror, DeckCardLink
|
||||
|
||||
### Critical Issues
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| C1 | `UserResponse` | **Missing fields from User model** | `avatar_bmp`, `salt`, `ban_ends`, `vip_expiry` are in the `User` model but absent from the response schema. `password_hash` is correctly excluded (security), but the others should be present or explicitly excluded. |
|
||||
|
||||
### Warnings
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| W1 | `schemas.py` (all) | **Pydantic v1 `class Config` used** | All schemas use `class Config: from_attributes = True` (Pydantic v1 style). Other schema files use Pydantic v2 `model_config = ConfigDict(from_attributes=True)`. Inconsistent. |
|
||||
| W2 | `GameResponse` | **Fields not in Room model** | `with_password`, `max_players`, `player_count`, `started` are not columns in the `Room` model. These appear to be computed/derived fields but are not documented. If computed, they should be `Field(default=...)` with defaults. |
|
||||
|
||||
### Info
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| I1 | `BanResponse` | **Missing `ip_address` field** | The `Ban` model has `ip_address` (String(45)) but `BanResponse` does not include it. |
|
||||
|
||||
---
|
||||
|
||||
## 2. `user_data_schemas.py` — ⚠️ FAIL
|
||||
|
||||
**Models covered:** UserSession, DeckVersion, GameReplay, ReplayPlayer, GameOutcome, UserStatistics, UserCardCollection, CardWishlist, UserGroup, GroupMember, GroupChatMessage, UserNetwork, NetworkMember, UserPreference, UserActivityLog
|
||||
|
||||
### Critical Issues
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| C1 | `GameReplayResponse` | **Missing `players` field** | The `GameReplay` model has a `players` relationship (List[ReplayPlayer]). The schema has `players: List[Dict[str, Any]] = []` which is a generic placeholder, not a typed schema. Should define a `ReplayPlayerResponse` schema. |
|
||||
|
||||
### Warnings
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| W1 | `DeckVersionStatus` enum | **Values don't match model** | Enum has `DRAFT`, `FINAL`, `ARCHIVED`. The `DeckVersion` model has `status` column with `server_default="DRAFT"` and `String(20)`. The `user_deck_schemas.py` `DeckStatus` enum has only `DRAFT`/`FINAL`. The `ARCHIVED` value has no model support. |
|
||||
| W2 | `GameReplayCreate` / `GameReplayUpdate` | **`format` is a Python keyword** | Using `format` as a field name shadows the built-in `format()` function. Should use `game_format` or `deck_format` instead. |
|
||||
| W3 | `GroupMember` model | **Missing `joined_at` in GroupMemberCreate** | `GroupMemberCreate` has `user_id` and `role` but the model also has `joined_at` (auto-set by server_default, so OK for create). Not a bug, but worth noting. |
|
||||
|
||||
### Info
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| I1 | `UserSessionResponse` | **Missing `session_token_hash`** | Model has `session_token_hash` but it's correctly excluded from response (security). Document this exclusion. |
|
||||
| I2 | `GroupResponse` | **`member_count` and `is_member` are computed** | Not in the `UserGroup` model. These are computed fields. Should have `Field(default=0)` / `Field(default=False)` with documentation. |
|
||||
| I3 | `NetworkResponse` | **Missing `updated_at`** | The `UserNetwork` model does NOT have `updated_at`, so this is actually correct. No issue. |
|
||||
|
||||
---
|
||||
|
||||
## 3. `user_deck_schemas.py` — ⚠️ FAIL
|
||||
|
||||
**Models covered:** UserDeck, UserDeckCard, DeckPrecedent, CardSuggestion
|
||||
|
||||
### Critical Issues
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| C1 | `UserDeckCreate` | **Missing `status` field** | The `UserDeck` model has `status` (String(20), default="DRAFT") as a required column. `UserDeckCreate` does not include `status`. If the default is relied upon, this is acceptable, but it should be explicit. |
|
||||
|
||||
### Warnings
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| W1 | `CardSearchResponse` | **Uses `List[Dict[str, Any]]` instead of typed schema** | Should use `List[CardResponse]` (from `card_search_schemas.py`) for type safety. |
|
||||
| W2 | `PrecedentResponse` | **Missing `updated_at` field** | The `DeckPrecedent` model has `updated_at` (DateTime) but `PrecedentResponse` does not include it. |
|
||||
| W3 | `DeckStatus` enum | **Conflicts with `user_data_schemas.py` `DeckVersionStatus`** | Both enums have `DRAFT`/`FINAL` values but different names and different files. The `DeckVersion` model in `user_data.py` uses `DeckVersionStatus` from `user_data_schemas.py`, while `UserDeck` model uses string values directly. This creates confusion. |
|
||||
|
||||
### Info
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| I1 | `DeckCardResponse` | **Missing `updated_at`** | The `UserDeckCard` model does NOT have `updated_at`, so this is correct. No issue. |
|
||||
| I2 | `SuggestionResponse` | **`card_name` is computed** | Not in the `CardSuggestion` model. Should be documented as a computed field with `Field(default="")`. |
|
||||
|
||||
---
|
||||
|
||||
## 4. `card_import_schemas.py` — ⚠️ FAIL
|
||||
|
||||
**Models covered:** UserCardImport, CardImportBatch
|
||||
|
||||
### Warnings
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| W1 | `CardImportStatusResponse` | **Missing fields vs CardImportBatch model** | Missing `batch_id`, `status`, `total_cards`, `matched_cards`, `unmatched_cards`, `error_message`. The `CardImportBatch` model has all these columns. |
|
||||
| W2 | `CardImportResponse` | **Missing `batch_id` and `status`** | The `CardImportBatch` model has `batch_id` (id) and `status` fields. Response only has `message`, `card_count`, `card_names`, `imported_at`. |
|
||||
|
||||
### Info
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| I1 | `CardMatchResult.match_type` | **Inconsistent with `card_search_schemas.py`** | This file uses `'exact'`, `'fuzzy'`, `'partial'`. The `card_search_schemas.py` version uses `'exact'`, `'high_confidence'`, `'low_confidence'`. |
|
||||
| I2 | `ErrorResponse` | **Duplicate of generic schemas** | Same as `MessageResponse`/`CountResponse` duplicates found in other files. |
|
||||
|
||||
---
|
||||
|
||||
## 5. `user_card_collection.py` — ⚠️ FAIL
|
||||
|
||||
**Models covered:** UserCardCollection, CardWishlist
|
||||
|
||||
### Warnings
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| W1 | `CardCollectionListResponse` | **Missing pagination fields** | Has `cards`, `total`, `page`, `page_size`, `total_pages` — this is actually correct and matches the pattern. No issue here. The warning is that `WishlistListResponse` is missing `page`/`page_size`/`total_pages` while `CardCollectionListResponse` has them. Inconsistent pagination. |
|
||||
|
||||
### Info
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| I1 | `CardCondition` / `AcquisitionMethod` enums | **Not validated against model** | The model stores these as `String(20)` and `String(50)` respectively. The enums provide validation at the schema layer, which is good. However, the `CardCollectionResponse` serializes them as plain `str`, losing the enum type info. |
|
||||
| I2 | `WishlistResponse` | **Missing `updated_at`** | The `CardWishlist` model does NOT have `updated_at`, so this is correct. No issue. |
|
||||
|
||||
---
|
||||
|
||||
## 6. `card_search_schemas.py` — ⚠️ FAIL
|
||||
|
||||
**Models covered:** MtgCard, MtgSet, CardImportBatch (partial)
|
||||
|
||||
### Critical Issues
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| C1 | `CardResponse` | **Missing 8 fields from MtgCard model** | Missing: `artist`, `flavor_text`, `numbers`, `image`, `card_parts`, `keywords`, `legalities`, `identifiers` (as typed). The `MtgCard` model has all these columns. |
|
||||
| C2 | `SetResponse` | **Missing 10 fields from MtgSet model** | Missing: `type`, `base_set_size`, `total_size`, `is_foil_only`, `is_non_foil_only`, `digital`, `icon_svg_url`, `parent_code`, `mtgo_code`, `image`. The `MtgSet` model has all these columns. |
|
||||
|
||||
### Warnings
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| W1 | `CardImportStatusResponse` | **Missing fields vs CardImportBatch** | Missing `batch_id`, `status`, `total_cards`, `matched_cards`, `unmatched_cards`, `error_message`. |
|
||||
| W2 | `CardMatchResult.match_type` | **Inconsistent with `card_import_schemas.py`** | Uses `'exact'`, `'high_confidence'`, `'low_confidence'` vs `'exact'`, `'fuzzy'`, `'partial'` in `card_import_schemas.py`. |
|
||||
| W3 | `CardImportResponse` | **Missing `file_type` and `file_size`** | The `CardImportBatch` model has `file_type` and `file_size` columns not reflected in this response. |
|
||||
|
||||
### Info
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| I1 | `CardSearchResponse` | **Uses `List[Dict[str, Any]]`** | Should use `List[CardResponse]` for type safety. |
|
||||
| I2 | `CardTypeResponse` | **Trivial schema** | Only has `type: str`. May be unnecessary or could be merged. |
|
||||
|
||||
---
|
||||
|
||||
## 7. `__init__.py` — ⚠️ FAIL
|
||||
|
||||
### Critical Issues
|
||||
|
||||
| # | Schema | Issue | Detail |
|
||||
|---|---|---|---|
|
||||
| C1 | `__init__.py` | **No imports — schemas not exported** | The file only contains `# Schemas package` comment. No schemas are imported or re-exported. Consumers must know the exact file path for each schema. Should at minimum export commonly used schemas. |
|
||||
|
||||
---
|
||||
|
||||
## Cross-File Issues
|
||||
|
||||
### Duplicate Generic Schemas (Found in 5 files)
|
||||
|
||||
The following generic schemas are defined identically in multiple files:
|
||||
|
||||
| Schema | Files |
|
||||
|---|---|
|
||||
| `MessageResponse` | `user_data_schemas.py`, `user_deck_schemas.py`, `card_import_schemas.py`, `user_card_collection.py`, `card_search_schemas.py` |
|
||||
| `CountResponse` | `user_data_schemas.py`, `user_deck_schemas.py`, `card_import_schemas.py`, `user_card_collection.py`, `card_search_schemas.py` |
|
||||
| `ErrorResponse` | `card_import_schemas.py`, `card_search_schemas.py` |
|
||||
| `ErrorDetail` | `user_data_schemas.py`, `user_card_collection.py` |
|
||||
|
||||
**Recommendation:** Create a `app/schemas/common.py` with these shared schemas and import from there.
|
||||
|
||||
### Models Without Any Schema Coverage
|
||||
|
||||
| Model | File |
|
||||
|---|---|
|
||||
| `UserCardImport` | `user_card_import.py` |
|
||||
| `UserCardImportRecord` | `user_card_import_record.py` |
|
||||
| `ReplayPlayer` | `user_data.py` |
|
||||
| `GroupMember` (response) | `user_data.py` |
|
||||
| `NetworkMember` (response) | `user_data.py` |
|
||||
| `DeckPrecedentCard` | `user_deck.py` |
|
||||
|
||||
### Pydantic v1 vs v2 Inconsistency
|
||||
|
||||
- `schemas.py`: Uses `class Config: from_attributes = True` (v1 style)
|
||||
- All other files: Use Pydantic v2 `BaseModel` (no explicit `model_config`, relying on defaults or not setting `from_attributes`)
|
||||
|
||||
**Recommendation:** Standardize on Pydantic v2 `model_config = ConfigDict(from_attributes=True)` in a common base or in each file.
|
||||
|
||||
---
|
||||
|
||||
## Recommendations (Priority Order)
|
||||
|
||||
1. **Fix `__init__.py`** — Add schema exports so consumers can import from `app.schemas`
|
||||
2. **Create `app/schemas/common.py`** — Extract duplicate `MessageResponse`, `CountResponse`, `ErrorResponse`, `ErrorDetail`
|
||||
3. **Fix `CardResponse` and `SetResponse`** in `card_search_schemas.py` — Add all missing model fields
|
||||
4. **Fix `CardImportStatusResponse`** in both `card_import_schemas.py` and `card_search_schemas.py` — Add missing fields
|
||||
5. **Standardize Pydantic v2** — Use `model_config = ConfigDict(from_attributes=True)` consistently
|
||||
6. **Resolve enum conflicts** — `DeckVersionStatus` vs `DeckStatus`, `CardMatchResult.match_type` values
|
||||
7. **Add `ReplayPlayerResponse`** — Replace `List[Dict[str, Any]]` in `GameReplayResponse`
|
||||
8. **Add missing schemas** — `UserCardImport`, `UserCardImportRecord`, `DeckPrecedentCard`
|
||||
9. **Fix `format` field name** in `GameReplayCreate`/`Update` — Rename to `game_format`
|
||||
10. **Add `updated_at`** to `PrecedentResponse` and `UserDeckCreate` (status field)
|
||||
Reference in New Issue
Block a user