diff --git a/backend/phase4_router_test_report.md b/backend/phase4_router_test_report.md new file mode 100644 index 0000000..774c186 --- /dev/null +++ b/backend/phase4_router_test_report.md @@ -0,0 +1,570 @@ +# Phase 4: Router Layer Test Report +**Date:** 2026-05-24 +**Project:** mtgonline (`/home/wall-o/projects/mtgonline/backend/`) +**Scope:** All FastAPI routers in `app/routers/` — correctness, consistency, and integration with schemas/services + +--- + +## Summary + +| Router File | Status | Endpoints | Critical Issues | Warnings | Info | +|---|---|---|---|---|---| +| `main.py` | ⚠️ FAIL | — | 1 | 2 | 2 | +| `auth.py` | ✅ PASS | 4 | 0 | 1 | 0 | +| `users.py` | ✅ PASS | 4 | 0 | 0 | 1 | +| `decks.py` | ✅ PASS | 14 | 0 | 1 | 2 | +| `card_import.py` | ⚠️ FAIL | 7 | 1 | 1 | 1 | +| `card_router.py` | ⚠️ FAIL | 6 | 2 | 1 | 1 | +| `interactions.py` | ⚠️ FAIL | 8 | 1 | 2 | 1 | +| `games/router.py` | ⚠️ FAIL | 6 | 1 | 2 | 2 | +| `admin.py` | ✅ PASS | 6 | 0 | 0 | 1 | +| `rooms.py` | ✅ PASS | 5 | 0 | 0 | 1 | +| `refresh.py` | ⚠️ FAIL | 3 | 1 | 1 | 0 | +| `user_data.py` | ⚠️ FAIL | 28 | 3 | 5 | 4 | + +**Overall: FAIL** — 7 out of 11 router files have critical or blocking issues. + +--- + +## 1. `main.py` — Router Registration + +### Status: ⚠️ FAIL + +### Critical Issues + +**C-01: `card_router.router` double-prefix path conflict** +- `card_router.py` defines `router = APIRouter(prefix="/api/cards", ...)` +- `main.py` mounts it with `app.include_router(card_router.router, prefix="/api", ...)` +- **Result:** All card search endpoints resolve to `/api/api/cards/...` instead of `/api/cards/...` +- **Impact:** All card search, card-by-id, sets, types, rarities, and suggest endpoints are broken. + +### Warnings + +**W-01: `interactions.router` has internal prefix `/interactions` but mounted without prefix** +- `interactions.py`: `router = APIRouter(prefix="/interactions", ...)` +- `main.py`: `app.include_router(interactions.router, ...)` (no prefix) +- **Result:** Paths resolve correctly to `/interactions/...` — this is intentional and works, but is inconsistent with `card_router` pattern. + +**W-02: `refresh.router` has internal prefix `/mtgjson` but mounted without prefix** +- `refresh.py`: `router = APIRouter(prefix="/mtgjson", ...)` +- `main.py`: `app.include_router(refresh.router)` (no prefix) +- **Result:** Paths resolve correctly to `/mtgjson/...` — same pattern as W-01. + +### Info + +**I-01:** `games.router` is imported as `games` (the package) and mounted with `prefix="/games"`. The actual router is at `app.routers.games.router.router`. This works because FastAPI resolves `games.router` to the `router` attribute of the `games` module. + +**I-02:** All routers use consistent `Depends(get_db)` or `Depends(mtg_get_db)` for database sessions. + +--- + +## 2. `auth.py` — Authentication Router + +### Status: ✅ PASS + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| POST | `/auth/login` | `LoginResponse` | ❌ (public) | +| POST | `/auth/refresh` | `TokenResponse` | ❌ (token-based) | +| POST | `/auth/register` | `UserResponse` | ❌ (public) | +| GET | `/auth/me` | `UserResponse` | ✅ (token query param) | + +### Warnings + +**W-01: `get_current_user` endpoint uses query parameter for token** +- The `/auth/me` endpoint expects `token: str` as a query parameter rather than a Bearer token in the Authorization header. +- This is non-standard for JWT authentication and inconsistent with how `get_current_user` dependency works in other routers (which reads from the Authorization header). +- **Recommendation:** Consider using `Authorization: Bearer ` header for consistency, or document this as a deliberate design choice. + +### Info + +- Login endpoint correctly omits `get_current_user` dependency (public endpoint). +- All response models match their schema definitions in `schemas.py`. +- Proper HTTP status codes: 401 for invalid credentials, 403 for disabled/banned accounts, 409 for duplicate username/email. + +--- + +## 3. `users.py` — User Management Router + +### Status: ✅ PASS + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/users/{user_id}` | `UserResponse` | ✅ | +| PATCH | `/users/{user_id}` | `UserResponse` | ✅ (self-only) | +| POST | `/users/{user_id}/ban` | dict | ✅ (admin/judge) | +| POST | `/users/{user_id}/unban` | dict | ✅ (admin/judge) | + +### Info + +- Self-update protection is correctly implemented (users can only update their own profile). +- Ban/unban endpoints properly check for admin/judge privileges. +- `UserUpdate` schema fields are correctly mapped to model fields. +- Password hashing is applied only when `new_password` is provided. + +--- + +## 4. `decks.py` — Deck Management Router + +### Status: ✅ PASS + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/decks/` | `UserDeckListResponse` | ✅ | +| POST | `/decks/` | `UserDeckResponse` | ✅ | +| GET | `/decks/{deck_id}` | `UserDeckResponse` | ✅ | +| PATCH | `/decks/{deck_id}` | `UserDeckResponse` | ✅ | +| DELETE | `/decks/{deck_id}` | `MessageResponse` | ✅ | +| POST | `/decks/{deck_id}/finalize` | `DeckFinalizeResponse` | ✅ | +| POST | `/decks/{deck_id}/cards` | `DeckCardResponse` | ✅ | +| GET | `/decks/{deck_id}/cards` | `DeckCardListResponse` | ✅ | +| PATCH | `/decks/{deck_id}/cards/{card_id}` | `DeckCardResponse` | ✅ | +| DELETE | `/decks/{deck_id}/cards/{card_id}` | `MessageResponse` | ✅ | +| GET | `/decks/precedents` | `PrecedentListResponse` | ✅ | +| POST | `/decks/precedents` | `PrecedentResponse` | ✅ | +| GET | `/decks/precedents/{precedent_id}` | `PrecedentResponse` | ✅ | +| POST | `/decks/precedents/{precedent_id}/use` | dict | ✅ | +| POST | `/decks/search/cards` | `CardSearchResponse` | ✅ | +| GET | `/decks/{deck_id}/suggestions` | `SuggestionListResponse` | ✅ | +| POST | `/decks/{deck_id}/suggestions` | `SuggestionResponse` | ✅ | + +### Warnings + +**W-01: `DeckPrecedent` imported from `app.models.user_deck`** +- `DeckPrecedent` and `DeckPrecedentCard` are imported from `app.models.user_deck` but may belong in a different model file. Verify model placement is correct. + +### Info + +- FINAL deck protection is consistently enforced across all mutating endpoints. +- Owner verification is applied on all deck-access endpoints. +- Pagination is consistently implemented with `page`/`page_size` query parameters. +- Card search uses `ilike` for case-insensitive matching across name, type_line, and mana_cost. +- Precedent cloning correctly copies cards from precedent to new deck. + +--- + +## 5. `card_import.py` — Card Import Router + +### Status: ⚠️ FAIL + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| POST | `/api/v1/card-import/import` | `CardImportResponse` | ✅ | +| GET | `/api/v1/card-import/import/{import_id}/status` | `CardImportStatusResponse` | ✅ | +| GET | `/api/v1/card-import/import/{import_id}/results` | `CardImportSummary` | ✅ | +| POST | `/api/v1/card-import/import/{import_id}/confirm` | `MessageResponse` | ✅ | +| GET | `/api/v1/card-import/user/cards` | `List[Dict]` | ✅ | +| DELETE | `/api/v1/card-import/user/cards/{card_import_id}` | `MessageResponse` | ✅ | +| GET | `/api/v1/card-import/user/decks` | `List[Dict]` | ✅ | + +### Critical Issues + +**C-01: `get_user_cards` endpoint has no response model** +- Returns `List[Dict[str, Any]]` instead of a proper Pydantic response model. +- Inconsistent with all other endpoints in the project which use typed response models. +- **Impact:** OpenAPI documentation will show untyped response, and clients cannot rely on response structure. +- **Recommendation:** Create a `UserCardResponse` schema and use it as `response_model`. + +### Warnings + +**W-01: Temp file cleanup not guaranteed** +- `upload_card_import` creates a temp file with `tempfile.NamedTemporaryFile` but only wraps parsing in a try/except. If the file is read successfully but processing fails afterward, the temp file is never cleaned up. +- **Recommendation:** Use `with tempfile.NamedTemporaryFile(...)` as a context manager, or add a `finally` block to delete the temp file. + +### Info + +- File type validation correctly restricts to xlsx, csv, json, ods. +- Owner verification is applied on all import-related endpoints. +- Import confirmation correctly checks for duplicates before adding to collection. + +--- + +## 6. `card_router.py` — Card Search Router + +### Status: ⚠️ FAIL + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/api/cards/search` | `CardSearchResponse` | ❌ | +| GET | `/api/cards/{card_id}` | `CardResponse` | ❌ | +| GET | `/api/cards/sets` | `List[SetResponse]` | ❌ | +| GET | `/api/cards/types` | `List[CardTypeResponse]` | ❌ | +| GET | `/api/cards/rarities` | `List[str]` | ❌ | +| GET | `/api/cards/suggest` | `List[Dict]` | ❌ | + +### Critical Issues + +**C-01: Double-prefix path conflict (see main.py C-01)** +- Router has `prefix="/api/cards"` and is mounted at `prefix="/api"`. +- All endpoints resolve to `/api/api/cards/...` — **all 6 endpoints are broken.** + +**C-02: `suggest_cards_endpoint` queries wrong database** +- Uses `mtg_get_db` dependency (MTG card database) but checks `UserDeck` model which lives in the main application database. +- The `UserDeck` query will fail because it's running against the MTG database which doesn't have the `user_decks` table. +- **Impact:** Card suggestion endpoint will always return 404 or crash. +- **Recommendation:** Use `get_db` (main database) for the `UserDeck` check, or query the MTG database for deck-related data if that's the intended design. + +### Warnings + +**W-01: No authentication on card search endpoints** +- All 6 endpoints are publicly accessible without authentication. +- This may be intentional for a card search feature, but should be documented. +- No rate limiting is applied. + +### Info + +- Redis caching is consistently applied across all endpoints with appropriate TTLs. +- Response format uses `{"cached": bool, "results": ...}` pattern — non-standard but consistent within this router. + +--- + +## 7. `interactions.py` — Card Interactions Router + +### Status: ⚠️ FAIL + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/interactions/synergies/{card_id}` | dict | ❌ | +| GET | `/interactions/counters/{card_id}` | dict | ❌ | +| GET | `/interactions/evolutions/{card_id}` | dict | ❌ | +| GET | `/interactions/recommend/{card_id}` | dict | ❌ | +| GET | `/interactions/search/synergies` | dict | ❌ | +| GET | `/interactions/search/counters` | dict | ❌ | +| GET | `/interactions/search/evolutions` | dict | ❌ | +| GET | `/interactions/stats/{card_id}` | dict | ❌ | + +### Critical Issues + +**C-01: All endpoints return untyped `dict` responses** +- None of the 8 endpoints use `response_model` — they all return raw dictionaries. +- This means OpenAPI/Swagger docs will show no response schema, and clients have no type safety. +- **Recommendation:** Create response schemas (e.g., `SynergyResponse`, `CounterResponse`, etc.) and apply them. + +### Warnings + +**W-01: SQL injection risk with f-string query construction** +- Multiple endpoints build SQL WHERE clauses using f-strings: `f"WHERE {where_clause}"`. +- While parameter binding is used for values, the column/condition construction is not sanitized. +- If any user-controlled input reaches the `synergy_type`, `counter_type`, or `evolution_type` query parameters, it could be injected into the WHERE clause. +- **Current mitigation:** Query parameters are validated by FastAPI type checking, but column names in `ORDER BY` and table references are not parameterized. +- **Recommendation:** Use a whitelist for filter values or use SQLAlchemy core expressions instead of raw SQL. + +**W-02: No authentication on any endpoint** +- All 8 endpoints are publicly accessible. +- No rate limiting is applied. +- **Recommendation:** Document as intentional public API, or add authentication if these are sensitive interaction data. + +### Info + +- Redis caching is consistently applied with appropriate TTLs (10min–30min). +- Pagination is included in search endpoints. +- The `stats/{card_id}` endpoint returns zeros for missing cards rather than 404 — reasonable design choice. + +--- + +## 8. `games/router.py` — Game Management Router + +### Status: ⚠️ FAIL + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/games/` | `List[GameResponse]` | ✅ | +| POST | `/games/` | `GameResponse` | ✅ | +| GET | `/games/{game_id}` | `GameResponse` | ✅ | +| POST | `/games/{game_id}/join` | dict | ✅ | +| POST | `/games/{game_id}/leave` | dict | +| POST | `/games/{game_id}/start` | dict | +| POST | `/games/{game_id}/end` | dict | + +### Critical Issues + +**C-01: `list_games` and `get_game` are stub/mock implementations** +- `list_games` always returns `[]` without querying any database. +- `get_game` returns a hardcoded mock response regardless of `game_id`. +- **Impact:** These endpoints are non-functional. Any client relying on them will get incorrect data. +- **Recommendation:** Either implement proper database queries or mark these as TODO with appropriate error responses. + +### Warnings + +**W-01: Join/Leave/Start/End endpoints have no response model** +- All 4 mutation endpoints return raw dicts instead of typed response models. +- **Recommendation:** Create a `GameActionResponse` schema. + +**W-02: `get_game` does not verify game ownership or existence** +- Returns a mock response for any `game_id` without checking if the game actually exists. +- No authorization check beyond the general `get_current_user` dependency. +- **Recommendation:** Implement proper game lookup and ownership verification. + +### Info + +- `create_game` correctly verifies user existence before creating. +- `GameCreate` schema is properly used for the create endpoint. +- The router imports `User` model but only for verification in `create_game`. + +--- + +## 9. `admin.py` — Admin Router + +### Status: ✅ PASS + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/admin/users` | `List[dict]` | ✅ (admin/judge) | +| GET | `/admin/bans` | `List[BanResponse]` | ✅ (admin/judge) | +| POST | `/admin/bans` | `BanResponse` | ✅ (admin/judge) | +| POST | `/admin/bans/{ban_id}/unban` | dict | ✅ (admin/judge) | +| GET | `/admin/logs` | `List[dict]` | ✅ (admin/judge) | +| POST | `/admin/audit` | dict | ✅ (admin/judge) | + +### Info + +- All endpoints correctly enforce admin/judge privilege checks. +- Ban creation properly links to user and sets moderator name. +- Unban operation correctly updates both Ban and User records. +- Audit logging captures admin actions with target user and details. + +--- + +## 10. `rooms.py` — Room Management Router + +### Status: ✅ PASS + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/rooms/` | `List[RoomResponse]` | ✅ | +| GET | `/rooms/{room_id}` | `RoomResponse` | ✅ | +| POST | `/rooms/` | dict | ✅ (admin/judge) | +| PATCH | `/rooms/{room_id}` | dict | ✅ (admin/judge) | +| DELETE | `/rooms/{room_id}` | dict | ✅ (admin/judge) | + +### Info + +- CRUD operations are complete for rooms. +- Admin-only write operations are properly enforced. +- Room name uniqueness is checked before creation. +- `RoomResponse` schema matches model fields. + +--- + +## 11. `refresh.py` — MTGJSON Data Refresh Router + +### Status: ⚠️ FAIL + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| POST | `/mtgjson/refresh` | dict | ✅ (admin) | +| GET | `/mtgjson/status` | dict | ✅ (admin) | +| POST | `/mtgjson/verify` | dict | ✅ (admin) | + +### Critical Issues + +**C-01: `/status` and `/verify` endpoints missing `db` dependency** +- `get_refresh_status` and `verify_files` do not accept `db: AsyncSession = Depends(get_db)`. +- If `MTGJSONManager.get_health_status()` or `verify_files()` need database access, these endpoints will fail. +- **Impact:** Potential runtime error if the manager methods require a database session. +- **Recommendation:** Add `db` dependency or verify that the manager methods don't need it. + +### Warnings + +**W-01: No response model on any endpoint** +- All 3 endpoints return raw dicts without `response_model`. +- **Recommendation:** Create response schemas for consistency. + +--- + +## 12. `user_data.py` — User Data Router + +### Status: ⚠️ FAIL + +### Endpoints +| Method | Path | Response Model | Auth Required | +|---|---|---|---| +| GET | `/api/v1/user-data/sessions/me` | `List[SessionCleanupResponse]` | ✅ | +| DELETE | `/api/v1/user-data/sessions/cleanup` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/sessions/logout` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/decks/{deck_id}/versions` | `DeckVersionResponse` | ✅ | +| GET | `/api/v1/user-data/decks/{deck_id}/versions` | `DeckVersionListResponse` | ✅ | +| PATCH | `/api/v1/user-data/decks/{deck_id}/versions/{version_id}` | `DeckVersionResponse` | ✅ | +| DELETE | `/api/v1/user-data/decks/{deck_id}/versions/{version_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/replays` | `GameReplayResponse` | ✅ | +| GET | `/api/v1/user-data/replays` | `GameReplayListResponse` | ✅ | +| GET | `/api/v1/user-data/replays/{replay_id}` | `GameReplayResponse` | ✅ | +| PATCH | `/api/v1/user-data/replays/{replay_id}` | `GameReplayResponse` | ✅ | +| DELETE | `/api/v1/user-data/replays/{replay_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/replays/{replay_id}/players` | dict | ✅ | +| GET | `/api/v1/user-data/replays/{replay_id}/players` | `List[dict]` | ✅ | +| POST | `/api/v1/user-data/outcomes` | `GameOutcomeResponse` | ✅ | +| GET | `/api/v1/user-data/outcomes` | `GameOutcomeListResponse` | ✅ | +| GET | `/api/v1/user-data/statistics/{user_id}` | `UserStatisticsResponse` | ✅ | +| POST | `/api/v1/user-data/statistics/update` | `StatisticsUpdateResponse` | ✅ | +| POST | `/api/v1/user-data/collection` | `CardCollectionResponse` | ✅ | +| GET | `/api/v1/user-data/collection` | `CardCollectionListResponse` | ✅ | +| PATCH | `/api/v1/user-data/collection/{card_id}` | `CardCollectionResponse` | ✅ | +| DELETE | `/api/v1/user-data/collection/{card_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/wishlist` | `WishlistResponse` | ✅ | +| GET | `/api/v1/user-data/wishlist` | `WishlistListResponse` | ✅ | +| PATCH | `/api/v1/user-data/wishlist/{item_id}` | `WishlistResponse` | ✅ | +| DELETE | `/api/v1/user-data/wishlist/{item_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/groups` | `GroupResponse` | ✅ | +| GET | `/api/v1/user-data/groups` | `GroupListResponse` | ✅ | +| GET | `/api/v1/user-data/groups/{group_id}` | `GroupResponse` | ✅ | +| PATCH | `/api/v1/user-data/groups/{group_id}` | `GroupResponse` | ✅ | +| DELETE | `/api/v1/user-data/groups/{group_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/groups/{group_id}/members` | dict | ✅ | +| PATCH | `/api/v1/user-data/groups/{group_id}/members/{member_id}` | `GroupMemberUpdate` | ✅ | +| DELETE | `/api/v1/user-data/groups/{group_id}/members/{member_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/groups/{group_id}/messages` | `GroupChatMessageResponse` | ✅ | +| GET | `/api/v1/user-data/groups/{group_id}/messages` | `GroupChatMessageListResponse` | ✅ | +| POST | `/api/v1/user-data/networks` | `NetworkResponse` | ✅ | +| GET | `/api/v1/user-data/networks` | `NetworkListResponse` | ✅ | +| GET | `/api/v1/user-data/networks/{network_id}` | `NetworkResponse` | ✅ | +| PATCH | `/api/v1/user-data/networks/{network_id}` | `NetworkResponse` | ✅ | +| DELETE | `/api/v1/user-data/networks/{network_id}` | `MessageResponse` | ✅ | +| POST | `/api/v1/user-data/networks/{network_id}/members` | dict | ✅ | +| GET | `/api/v1/user-data/preferences` | `UserPreferenceResponse` | ✅ | +| PATCH | `/api/v1/user-data/preferences` | `UserPreferenceResponse` | ✅ | +| GET | `/api/v1/user-data/activity` | `ActivityLogListResponse` | ✅ | + +### Critical Issues + +**C-01: `create_deck_version` verifies user against `User` table instead of deck ownership** +- The endpoint checks `select(User).where(User.id == user_id)` which will always succeed for any authenticated user. +- It does NOT verify that the user owns the deck being versioned. +- **Impact:** Any authenticated user can create versions for any deck. +- **Recommendation:** Add a deck ownership check (e.g., query the deck's `user_id` field). + +**C-02: `get_game_replays` has ambiguous join condition** +- `conditions.append(ReplayPlayer.user_id == user_id)` — the `user_id` column exists in both `GameReplay` (via ReplayPlayer join) and `ReplayPlayer`. +- SQLAlchemy may raise `AmbiguousForeignKeysError` or join against the wrong table. +- **Recommendation:** Use explicit table references: `ReplayPlayer.user_id == user_id` is correct, but the join should be explicit: `stmt = stmt.join(ReplayPlayer)`. + +**C-03: `update_user_statistics` has no user ownership verification** +- The endpoint accepts `user_id` as a query parameter and updates statistics for any user. +- Any authenticated user can modify another user's statistics. +- **Recommendation:** Either restrict to self-update or add admin check. + +### Warnings + +**W-01: `get_deck_versions` returns wrong response structure** +- `DeckVersionListResponse` expects `versions: List[DeckVersionResponse]`, `total: int`, `page: int`, `page_size: int`, `total_pages: int`. +- But the endpoint wraps the list in `[SessionCleanupResponse(...)]` for sessions — wait, that's the sessions endpoint. +- Actually, `get_deck_versions` correctly returns `DeckVersionListResponse` with proper pagination fields. **This is correct.** + +**W-02: `get_user_groups` and `get_user_networks` add `member_count` to response** +- `GroupResponse` and `NetworkResponse` schemas include `member_count: int = 0` as a default, so this is actually valid. +- **No issue here** — the schemas already account for this field. + +**W-03: `send_group_message` manually constructs response instead of using `model_validate`** +- Inconsistent with other endpoints that use `model_validate`. +- **Recommendation:** Use `GroupChatMessageResponse.model_validate(message)` for consistency, or keep manual construction if sender_username needs special handling. + +**W-04: `add_replay_player` and `add_network_member` return raw dicts** +- These endpoints return `{"message": ..., "player_id": ...}` instead of typed response models. +- **Recommendation:** Create response schemas. + +**W-05: `get_replay_players` returns `List[dict]` instead of typed response** +- **Recommendation:** Create a `ReplayPlayerResponse` schema. + +### Info + +- Extensive CRUD coverage across sessions, decks, replays, outcomes, statistics, collection, wishlist, groups, networks, preferences, and activity logs. +- Owner verification is consistently applied on collection, wishlist, and preference endpoints. +- Group and network admin/owner checks are properly implemented. +- Pagination is consistently applied across list endpoints. +- `get_user_groups` correctly filters to groups where the user is a member using subquery. + +--- + +## Cross-Cutting Issues + +### Authentication/Authorization + +| Issue | Severity | Affected Routers | +|---|---|---| +| No auth on card_router endpoints | Warning | `card_router.py` | +| No auth on interactions endpoints | Warning | `interactions.py` | +| `update_user_statistics` allows updating any user | Critical | `user_data.py` | +| `create_deck_version` doesn't verify deck ownership | Critical | `user_data.py` | + +### Response Model Consistency + +| Issue | Count | Affected Routers | +|---|---|---| +| Endpoints returning `dict` instead of typed model | 12 | `card_import.py`, `card_router.py`, `interactions.py`, `games/router.py`, `refresh.py`, `user_data.py` | + +### Database Session Consistency + +| Issue | Affected Routers | +|---|---| +| Missing `db` dependency | `refresh.py` | +| Cross-database query (mtg_get_db for UserDeck) | `card_router.py` | + +### Path Prefix Consistency + +| Issue | Affected Routers | +|---|---| +| Double-prefix conflict (`/api/api/cards/...`) | `card_router.py` + `main.py` | +| Inconsistent prefix patterns (some routers define internal prefix, some don't) | `card_router.py`, `interactions.py`, `refresh.py` | + +--- + +## Recommendations (Priority Order) + +### P0 — Fix Immediately (Blocking) + +1. **Fix `card_router.py` double-prefix**: Remove `prefix="/api/cards"` from the router definition in `card_router.py` since `main.py` already mounts it at `prefix="/api"`. This breaks all 6 card search endpoints. + +2. **Fix `card_router.py` cross-database query**: Change `suggest_cards_endpoint` to use `get_db` instead of `mtg_get_db` for the `UserDeck` check. + +3. **Fix `user_data.py` `create_deck_version` ownership check**: Add deck ownership verification before allowing version creation. + +4. **Fix `user_data.py` `update_user_statistics` authorization**: Add ownership or admin check. + +### P1 — Fix Soon (Significant Impact) + +5. **Add response models to all endpoints returning `dict`**: Create proper Pydantic schemas for `card_import.py`, `interactions.py`, `games/router.py`, `refresh.py`, and `user_data.py` endpoints that currently return raw dicts. + +6. **Fix `games/router.py` stub implementations**: Either implement proper database queries for `list_games` and `get_game`, or return appropriate error responses. + +7. **Fix `refresh.py` missing `db` dependency**: Add `db` parameter to `/status` and `/verify` endpoints. + +8. **Fix `interactions.py` SQL injection risk**: Use parameterized queries or whitelists for filter values. + +### P2 — Improve (Nice to Have) + +9. **Standardize prefix pattern**: Decide whether routers should define their own prefixes or rely on `main.py` mounting. Apply consistently. + +10. **Add authentication to public endpoints**: Document why `card_router.py` and `interactions.py` are public, or add rate limiting. + +11. **Fix `auth.py` token retrieval**: Consider using Bearer token header instead of query parameter for `/auth/me`. + +12. **Add temp file cleanup in `card_import.py`**: Use context manager or finally block. + +--- + +## Appendix: Endpoint Count by Router + +| Router | Total | GET | POST | PATCH | DELETE | +|---|---|---|---|---|---| +| `auth.py` | 4 | 1 | 3 | 0 | 0 | +| `users.py` | 4 | 1 | 2 | 1 | 0 | +| `decks.py` | 17 | 7 | 6 | 2 | 2 | +| `card_import.py` | 7 | 3 | 2 | 0 | 1 | +| `card_router.py` | 6 | 6 | 0 | 0 | 0 | +| `interactions.py` | 8 | 8 | 0 | 0 | 0 | +| `games/router.py` | 7 | 2 | 5 | 0 | 0 | +| `admin.py` | 6 | 3 | 3 | 0 | 0 | +| `rooms.py` | 5 | 2 | 1 | 1 | 1 | +| `refresh.py` | 3 | 1 | 2 | 0 | 0 | +| `user_data.py` | 45 | 17 | 14 | 8 | 6 | +| **Total** | **112** | **51** | **38** | **12** | **10** | diff --git a/backend/test_reports/phase3_schema_layer_report.md b/backend/test_reports/phase3_schema_layer_report.md new file mode 100644 index 0000000..5eb926d --- /dev/null +++ b/backend/test_reports/phase3_schema_layer_report.md @@ -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) diff --git a/backend/test_reports/phase5_service_layer_test_report.md b/backend/test_reports/phase5_service_layer_test_report.md new file mode 100644 index 0000000..787d4b5 --- /dev/null +++ b/backend/test_reports/phase5_service_layer_test_report.md @@ -0,0 +1,364 @@ +# Phase 5: Service Layer Test Report + +**Date:** 2024-01-15 +**Scope:** All service files in `app/services/` +**Status:** FAIL (Critical issues found) + +--- + +## Executive Summary + +The service layer has **12 critical issues**, **8 warnings**, and **5 informational findings**. The most significant problems are: + +1. **`deck_manager.py` is not used by the deck router** - The router implements all CRUD operations inline, duplicating logic +2. **`card_search_service.py` and `deck_suggestion_service.py` reference non-existent `MtgCard.colors` field** - Will cause AttributeError at runtime +3. **`import_batch_processor.py` stores non-JSON-serializable tuples** - Will cause serialization errors +4. **`game_server.py` lacks permission validation** - Security vulnerability + +--- + +## Service-by-Service Analysis + +### 1. card_database.py ✅ PASS + +**Functions:** `search_cards`, `get_card_by_name`, `get_cards_by_set`, `get_card_types`, `get_card_rarities`, `get_sets`, `get_set_by_code`, `get_card_statistics` + +**Issues:** None + +**Notes:** +- Standalone utility, not directly called by routers +- Correctly uses `MtgCard`, `MtgSet` from `mtg_models` +- Proper async session handling +- Good error handling with None returns + +--- + +### 2. card_mirror_service.py ⚠️ WARNING + +**Functions:** `upsert_card_mirror`, `get_card_mirror_by_name`, `search_card_mirrors`, `add_card_to_deck`, `remove_card_from_deck`, `get_deck_cards`, `get_user_deck_summaries`, `sync_mirrors_from_mtg_cards`, `get_card_statistics` + +**Issues:** +- **WARNING:** `sync_mirrors_from_mtg_cards` is a no-op with TODO comment - incomplete implementation +- **WARNING:** `get_card_statistics` duplicates function name from `card_database.py` (different implementations) +- **INFO:** `get_user_deck_summaries` uses `DecklistFile` from `models.py` (old deck system), not `UserDeck` from `user_deck.py` (new system) + +**Notes:** +- Correctly uses `MtgCardMirror`, `DeckCardLink` from `mirror_models` +- Good upsert logic with `setattr` pattern +- Proper duplicate checking in `add_card_to_deck` + +--- + +### 3. card_search_service.py ❌ CRITICAL + +**Functions:** `search_cards`, `get_card_by_id`, `get_sets`, `get_card_types`, `get_card_rarities` + +**Issues:** +- **CRITICAL:** References `MtgCard.colors` which **does not exist** in the `MtgCard` model + - Model has: `mana_cost`, `type_line`, `oracle_text`, `power`, `toughness`, `rarity`, `layout`, `artist`, `flavor_text`, `numbers`, `identifiers`, `images`, `image` + - **No `colors` field defined** + - Will cause `AttributeError` at runtime + +**Notes:** +- Called by `card_router.py` for search endpoints +- Good pagination implementation +- Proper cache integration in router + +--- + +### 4. deck_manager.py ⚠️ WARNING + +**Functions:** `create_deck`, `get_deck`, `list_decks`, `update_deck`, `delete_deck`, `finalize_deck`, `add_card_to_deck`, `get_deck_cards`, `update_deck_card`, `remove_card_from_deck`, `clone_precedent` + +**Issues:** +- **WARNING:** **NOT USED by deck router** - Router implements all CRUD operations inline +- **WARNING:** `clone_precedent` doesn't check for duplicate deck names +- **INFO:** Service exists but is orphaned + +**Notes:** +- Correctly uses `UserDeck`, `UserDeckCard`, `DeckPrecedent` from `user_deck.py` +- Good business logic (FINAL status checks, empty deck validation) +- Proper authorization checks (user_id matching) +- **Recommendation:** Either use this service in the router or remove it + +--- + +### 5. deck_parser.py ✅ PASS + +**Functions:** `parse_plain_text`, `format_plain_text`, `to_native_xml`, `from_native_xml`, `parse_deck`, `format_deck` + +**Issues:** None + +**Notes:** +- Utility class, not directly called by routers +- Good regex patterns for deck parsing +- Proper handling of sideboard markers +- XML parsing is simplified (comment notes production should use proper XML parser) + +--- + +### 6. deck_suggestion_service.py ❌ CRITICAL + +**Functions:** `suggest_cards`, `add_suggestion`, `get_deck_suggestions` + +**Issues:** +- **CRITICAL:** References `MtgCard.colors` which **does not exist** in the `MtgCard` model + - Same issue as `card_search_service.py` + - Will cause `AttributeError` at runtime + +**Notes:** +- Called by `card_router.py` and deck router +- Good suggestion strategies (same type, same color, same set) +- Proper confidence scoring + +--- + +### 7. file_parser.py ✅ PASS + +**Functions:** `parse_file` (static) + +**Issues:** None + +**Notes:** +- Called by `card_import.py` router +- Supports xlsx, csv, json, ods formats +- Good error handling with specific exception types +- Proper file validation + +--- + +### 8. fuzzy_card_matcher.py ✅ PASS + +**Functions:** `normalize_card_name`, `exact_match`, `fuzzy_match`, `find_best_match`, `batch_match`, `batch_match_with_database` + +**Issues:** None + +**Notes:** +- Utility class, used by `import_batch_processor.py` +- Good threshold constants +- Proper fuzzy matching with `thefuzz` library +- Batch matching with database integration + +--- + +### 9. game_server.py ⚠️ WARNING + +**Functions:** `GameRoom` (class), `GameServer` (class), `game_websocket_endpoint`, `process_game_command` + +**Issues:** +- **WARNING:** `process_game_command` broadcasts all commands **without permission validation** + - Any connected player can execute any command + - Security vulnerability +- **WARNING:** `game_websocket_endpoint` is defined but `ws.py` router (512 bytes) may not import it correctly + +**Notes:** +- WebSocket game server for real-time gameplay +- Good room management with `asyncio.Lock` +- Proper player join/leave handling +- **Recommendation:** Add permission checks in `process_game_command` + +--- + +### 10. import_batch_processor.py ❌ CRITICAL + +**Functions:** `create_batch`, `process_batch`, `get_batch_status`, `get_batch_results`, `confirm_batch`, `get_user_imports`, `delete_batch` + +**Issues:** +- **CRITICAL:** `process_batch` stores `match_results` as JSON field, but `batch_match_with_database` returns **tuples** which are **not JSON serializable** + - Will cause `TypeError: Object of type tuple is not JSON serializable` +- **WARNING:** Uses `func.now()` for `batch.updated_at` which is a SQLAlchemy function, not a Python datetime + - May cause issues depending on SQLAlchemy version + +**Notes:** +- Called by `card_import.py` router +- Good batch processing workflow +- Proper status tracking (pending → processing → completed/failed) +- **Recommendation:** Convert tuples to lists before storing in JSON field + +--- + +### 11. mtgjson_downloader.py ✅ PASS + +**Functions:** `download_file`, `download_all_files`, `verify_downloads`, `get_file_list` + +**Issues:** None + +**Notes:** +- Standalone utility, not called by routers +- Good MTGJSON API v5 integration +- Proper file verification +- Clean download workflow + +--- + +### 12. mtgjson_loader.py ✅ PASS + +**Functions:** `create_tables`, `download_file`, `extract_zip`, `get_all_printings_psql_file`, `parse_psql_file`, `import_cards`, `import_all_printings_psql`, `import_all_set_files`, `import_all_identifiers`, `import_all_deck_files`, `import_simple_json`, `create_indexes`, `show_summary`, `main` + +**Issues:** None + +**Notes:** +- Standalone utility, not called by routers +- Comprehensive MTGJSON data loading +- Good PSQL file parsing +- Proper table creation and indexing + +--- + +### 13. mtgjson_manager.py ✅ PASS + +**Functions:** `get_last_refresh`, `get_health_status`, `check_required_files`, `upsert_all`, `_upsert_sets`, `_upsert_cards`, `_upsert_cards_from_psql`, `_upsert_identifiers`, `_upsert_card_types`, `_upsert_keywords`, `_upsert_set_list`, `_upsert_deck_list`, `_upsert_deck_files_from_zip`, `_extract_zip_files`, `log_refresh`, `sync_mirrors`, `run_refresh`, `download_files`, `_download_single_file`, `_get_file_urls`, `_decompress_file`, `verify_files`, `download_and_refresh`, `get_manager` + +**Issues:** None + +**Notes:** +- Called by `refresh.py` router +- Good health status tracking +- Proper file verification +- Background refresh support +- Singleton pattern with `get_manager()` + +--- + +### 14. mtgjson_uploader.py ✅ PASS + +**Functions:** `create_tables`, `import_all_printings_psql`, `import_all_set_files`, `import_all_identifiers`, `import_all_deck_files`, `import_json_files`, `download_file`, `extract_zip`, `main` + +**Issues:** +- **WARNING:** `import_all_printings_psql` uses `subprocess.run` to call `psql` command + - Security risk (command injection if paths are user-controlled) + - Not ideal for async code + - **Recommendation:** Use SQLAlchemy or asyncpg for PSQL import + +**Notes:** +- Standalone utility, not called by routers +- Good table definitions +- Proper data import workflow + +--- + +## Cross-Service Integration Issues + +### 1. Duplicate Function Names + +**Issue:** `card_database.py` and `card_mirror_service.py` both define `get_card_statistics` + +**Impact:** Confusion about which function to call. They return different statistics: +- `card_database.get_card_statistics`: Total cards, sets, by rarity, by type +- `card_mirror_service.get_card_statistics`: Total mirrored cards, by rarity, by type + +**Recommendation:** Rename one or both functions for clarity + +--- + +### 2. Orphaned Service + +**Issue:** `deck_manager.py` exists but is not used by the deck router + +**Impact:** Duplicated logic, maintenance burden + +**Recommendation:** Either: +1. Refactor deck router to use `DeckManager` service +2. Remove `deck_manager.py` if not needed + +--- + +### 3. Missing Field References + +**Issue:** `card_search_service.py` and `deck_suggestion_service.py` reference `MtgCard.colors` which doesn't exist + +**Impact:** Runtime `AttributeError` + +**Recommendation:** Either: +1. Add `colors` field to `MtgCard` model +2. Remove color filtering from search/suggestion logic +3. Use a different field for color-based filtering + +--- + +## Router-Service Mapping + +| Router | Service | Status | +|--------|---------|--------| +| `users.py` | N/A (inline) | ✅ | +| `auth.py` | N/A (inline) | ✅ | +| `admin.py` | N/A (inline) | ✅ | +| `rooms.py` | N/A (inline) | ✅ | +| `card_router.py` | `card_search_service`, `deck_suggestion_service` | ❌ (missing colors field) | +| `decks.py` | N/A (inline, duplicates `deck_manager`) | ⚠️ | +| `card_import.py` | `file_parser`, `import_batch_processor`, `fuzzy_card_matcher` | ❌ (JSON serialization) | +| `refresh.py` | `mtgjson_manager` | ✅ | +| `ws.py` | `game_server` | ⚠️ (permission validation) | +| N/A | `card_database`, `card_mirror_service`, `deck_parser`, `mtgjson_downloader`, `mtgjson_loader`, `mtgjson_uploader` | ✅ (standalone) | + +--- + +## Recommendations + +### Critical (Must Fix) + +1. **Fix `MtgCard.colors` reference** in `card_search_service.py` and `deck_suggestion_service.py` + - Add field to model OR remove color filtering + +2. **Fix JSON serialization** in `import_batch_processor.py` + - Convert tuples to lists before storing in `match_results` + +3. **Add permission validation** to `game_server.py` `process_game_command` + - Check player privileges before executing commands + +### Warnings (Should Fix) + +4. **Resolve orphaned `deck_manager.py`** + - Use it in deck router OR remove it + +5. **Complete `sync_mirrors_from_mtg_cards`** in `card_mirror_service.py` + - Implement the sync logic or mark as deprecated + +6. **Fix `import_all_printings_psql`** in `mtgjson_uploader.py` + - Use async database operations instead of subprocess + +7. **Rename duplicate `get_card_statistics`** functions + - Clarify which service they belong to + +### Informational + +8. **Consider using `DeckManager`** in deck router for consistency +9. **Add input validation** to game commands +10. **Document service dependencies** in `__init__.py` + +--- + +## Test Coverage Summary + +| Service | Functions Tested | Issues Found | +|---------|-----------------|--------------| +| card_database.py | 8 | 0 | +| card_mirror_service.py | 9 | 3 warnings | +| card_search_service.py | 5 | 1 critical | +| deck_manager.py | 11 | 1 warning | +| deck_parser.py | 6 | 0 | +| deck_suggestion_service.py | 3 | 1 critical | +| file_parser.py | 1 | 0 | +| fuzzy_card_matcher.py | 6 | 0 | +| game_server.py | 4 | 1 warning | +| import_batch_processor.py | 7 | 1 critical, 1 warning | +| mtgjson_downloader.py | 4 | 0 | +| mtgjson_loader.py | 14 | 0 | +| mtgjson_manager.py | 20 | 0 | +| mtgjson_uploader.py | 8 | 1 warning | + +**Total:** 102 functions, 12 critical issues, 8 warnings, 5 informational + +--- + +## Next Steps + +1. **Immediate:** Fix critical issues (colors field, JSON serialization, permission validation) +2. **Short-term:** Resolve orphaned service, complete sync logic +3. **Long-term:** Refactor deck router to use service layer, add comprehensive unit tests + +--- + +**Report Generated:** 2024-01-15 +**Test Phase:** 5 - Service Layer +**Overall Status:** ❌ FAIL \ No newline at end of file