cal/paper-dynasty-database
1
0
Fork 0
Code Issues 7 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

fix: remove dead roster fields from CSV in v1_cards_get_one (#25) #35

Merged
cal merged 1 commits from ai/paper-dynasty-database-25 into next-release 2026-03-03 21:45:16 +00:00
Conversation 1 Commits 1 Files Changed 1 +2 -3
cal commented 2026-03-03 18:06:35 +00:00
Owner
Copy Link

Summary

The v1_cards_get_one endpoint's CSV branch referenced this_card.roster1.name, this_card.roster2.name, and this_card.roster3.name — none of which exist on the Card model. Accessing these would raise an AttributeError at runtime for any request with ?csv=true.

Fix

Removed the three non-existent roster columns from both the CSV header row and data row. The CSV output now correctly reflects only the fields that Card actually has: id, player, team, pack, value.

Files Changed

  • app/routers_v2/cards.py — lines 144–146: removed roster1, roster2, roster3 from CSV header and data

Test Results

No test suite. Fix verified by code inspection — Card model in db_engine.py confirms fields are player, team, pack, value only.

Other Observations

  • The v1_cards_patch endpoint still accepts roster1_id, roster2_id, roster3_id query parameters but the Card model has no such fields either. Those params appear to be silently ignored (no assignment to this_card), so they don't cause a runtime error — but they're dead API surface worth cleaning up separately.
## Summary The `v1_cards_get_one` endpoint's CSV branch referenced `this_card.roster1.name`, `this_card.roster2.name`, and `this_card.roster3.name` — none of which exist on the `Card` model. Accessing these would raise an `AttributeError` at runtime for any request with `?csv=true`. ## Fix Removed the three non-existent roster columns from both the CSV header row and data row. The CSV output now correctly reflects only the fields that `Card` actually has: `id`, `player`, `team`, `pack`, `value`. ## Files Changed - `app/routers_v2/cards.py` — lines 144–146: removed `roster1`, `roster2`, `roster3` from CSV header and data ## Test Results No test suite. Fix verified by code inspection — `Card` model in `db_engine.py` confirms fields are `player`, `team`, `pack`, `value` only. ## Other Observations - The `v1_cards_patch` endpoint still accepts `roster1_id`, `roster2_id`, `roster3_id` query parameters but the `Card` model has no such fields either. Those params appear to be silently ignored (no assignment to `this_card`), so they don't cause a runtime error — but they're dead API surface worth cleaning up separately.
cal added 1 commit 2026-03-03 18:06:36 +00:00
fix: remove dead roster fields from CSV in v1_cards_get_one (#25)
All checks were successful
Build Docker Image / build (pull_request) Successful in 3m16s
Details
77448200b6 
Card model has no roster1/2/3 fields. Accessing them would raise
AttributeError at runtime. Removed the non-existent columns from
the CSV header and data row.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
tech-debt
 label 2026-03-03 18:33:52 +00:00
cal reviewed 2026-03-03 18:35:19 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/cards.py (modified)

Findings

Correctness

  • Fix is accurate. Card model (db_engine.py lines 472–491) confirms it defines only player, team, pack, and value fields — no roster1, roster2, roster3. The removed attributes would have raised AttributeError on every GET /api/v2/cards/{id}?csv=true request.
  • The remaining CSV fields (id, player, this_card.team.abbrev, pack, value) are all valid — consistent with the Card model's actual FK relationships.
  • Diff is minimal and surgical: only the broken lines are removed, no unrelated changes.

Security

  • No issues found. No new inputs, no new query paths.

Style & Conventions

  • Follows existing code style. No issues.

Suggestions

  • The PR author correctly flagged that v1_cards_patch (lines 302–348) still accepts roster1_id, roster2_id, roster3_id params and assigns them to this_card (lines 332–337). Since these aren't real Card model fields, Peewee silently drops them on save() — no runtime error, but it is dead API surface. Worth a follow-up cleanup issue.

Verdict: APPROVED

The fix correctly removes three non-existent attribute accesses that would have crashed the CSV branch of v1_cards_get_one. The change is correct, focused, and safe to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/cards.py` (modified) ### Findings #### Correctness - Fix is accurate. `Card` model (`db_engine.py` lines 472–491) confirms it defines only `player`, `team`, `pack`, and `value` fields — no `roster1`, `roster2`, `roster3`. The removed attributes would have raised `AttributeError` on every `GET /api/v2/cards/{id}?csv=true` request. - The remaining CSV fields (`id`, `player`, `this_card.team.abbrev`, `pack`, `value`) are all valid — consistent with the `Card` model's actual FK relationships. - Diff is minimal and surgical: only the broken lines are removed, no unrelated changes. #### Security - No issues found. No new inputs, no new query paths. #### Style & Conventions - Follows existing code style. No issues. #### Suggestions - The PR author correctly flagged that `v1_cards_patch` (lines 302–348) still accepts `roster1_id`, `roster2_id`, `roster3_id` params and assigns them to `this_card` (lines 332–337). Since these aren't real `Card` model fields, Peewee silently drops them on `save()` — no runtime error, but it is dead API surface. Worth a follow-up cleanup issue. ### Verdict: APPROVED The fix correctly removes three non-existent attribute accesses that would have crashed the CSV branch of `v1_cards_get_one`. The change is correct, focused, and safe to merge. --- *Automated review by Claude PR Reviewer*
cal added the
ai-reviewed
 label 2026-03-03 18:35:47 +00:00
cal commented 2026-03-03 19:06:02 +00:00
Author
Owner
Copy Link

Fixed in PR #36: #36

Removed roster1, roster2, roster3 from both the CSV header and data rows in v1_cards_get_one. These fields don't exist on the Card model and would cause AttributeError on any ?csv=true request.

Fixed in PR #36: https://git.manticorum.com/cal/paper-dynasty-database/pulls/36 Removed `roster1`, `roster2`, `roster3` from both the CSV header and data rows in `v1_cards_get_one`. These fields don't exist on the `Card` model and would cause `AttributeError` on any `?csv=true` request.
cal added the
ai-pr-opened
 label 2026-03-03 19:06:04 +00:00
cal changed target branch from main to next-release 2026-03-03 21:44:42 +00:00
cal force-pushed ai/paper-dynasty-database-25 from 77448200b6 to 6a2e8a2d2a 2026-03-03 21:45:02 +00:00 Compare
cal merged commit 215c48a53e into next-release 2026-03-03 21:45:16 +00:00
cal deleted branch ai/paper-dynasty-database-25 2026-03-03 21:45:16 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working

   
bug

   
enhancement

   
evolution

Card Evolution system

  
performance

   
phase-0

Render Pipeline Optimization

  
phase-1a

Schema & Data Foundation

  
phase-1b

API & Formula Engine

  
phase-1c

Bot Integration

  
phase-1d

Deployment

  
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
tech-debt
todo
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#35
No description provided.
Delete Branch "ai/paper-dynasty-database-25"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?