mtgonline/backend/phase4_router_test_report.md

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 <token> 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