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

fix: remove legacy SQLite compatibility code (#122) #126

Open
Claude wants to merge 1 commits from ai/paper-dynasty-database#122 into main
pull from: ai/paper-dynasty-database#122
merge into: cal:main
Conversation 0 Commits 1 Files Changed 5 +44 -331
Claude commented 2026-03-23 04:37:31 +00:00
Collaborator
Copy Link

Closes #122

Summary

  • app/db_engine.py: Removes DATABASE_TYPE env var, SKIP_TABLE_CREATION, the SQLite else branch, all 13 db.create_tables() guards, and commented-out SQLite scout_db code (~280 lines removed). PooledPostgresqlDatabase is now initialized unconditionally.
  • app/db_helpers.py: Removes DATABASE_TYPE re-export and the SQLite on_conflict_replace() branch from upsert_many(). PostgreSQL ON CONFLICT DO UPDATE is now the only upsert path.
  • app/routers_v2/players.py: Updates stale comment referencing SQLite compatibility.
  • tests/conftest.py: Removes os.environ["DATABASE_TYPE"] = "postgresql" (no longer needed); keeps dummy POSTGRES_PASSWORD for instantiation.
  • CLAUDE.md: Updates description, ORM line, Key Env Vars, and removes SQLite locking common issue.

Other observations

  • tests/test_evolution_seed.py and tests/test_season_stats_model.py use SqliteDatabase(':memory:') for unit test isolation — these are not production SQLite compatibility code and are intentionally left in place.
  • Pre-existing Pyright warnings in players.py (unrelated to this change).

Test results

No test command configured. Changes verified by reading back modified files and confirming no remaining sqlite, DATABASE_TYPE, SKIP_TABLE_CREATION, or pd_master.db references in application code.

Closes #122 ## Summary - **`app/db_engine.py`**: Removes `DATABASE_TYPE` env var, `SKIP_TABLE_CREATION`, the SQLite `else` branch, all 13 `db.create_tables()` guards, and commented-out SQLite `scout_db` code (~280 lines removed). `PooledPostgresqlDatabase` is now initialized unconditionally. - **`app/db_helpers.py`**: Removes `DATABASE_TYPE` re-export and the SQLite `on_conflict_replace()` branch from `upsert_many()`. PostgreSQL `ON CONFLICT DO UPDATE` is now the only upsert path. - **`app/routers_v2/players.py`**: Updates stale comment referencing SQLite compatibility. - **`tests/conftest.py`**: Removes `os.environ["DATABASE_TYPE"] = "postgresql"` (no longer needed); keeps dummy `POSTGRES_PASSWORD` for instantiation. - **`CLAUDE.md`**: Updates description, ORM line, Key Env Vars, and removes SQLite locking common issue. ## Other observations - `tests/test_evolution_seed.py` and `tests/test_season_stats_model.py` use `SqliteDatabase(':memory:')` for unit test isolation — these are not production SQLite compatibility code and are intentionally left in place. - Pre-existing Pyright warnings in `players.py` (unrelated to this change). ## Test results No test command configured. Changes verified by reading back modified files and confirming no remaining `sqlite`, `DATABASE_TYPE`, `SKIP_TABLE_CREATION`, or `pd_master.db` references in application code.
Claude added 1 commit 2026-03-23 04:37:32 +00:00
fix: remove legacy SQLite compatibility code (#122)
All checks were successful
Build Docker Image / build (pull_request) Successful in 8m32s
Details
ec04a111c5 
Closes #122

Both prod and dev environments use PostgreSQL. Removes all SQLite
compatibility code that was never exercised in practice.

Changes:
- db_engine.py: replace SQLite/PostgreSQL branching with direct
  PooledPostgresqlDatabase init; remove DATABASE_TYPE, SKIP_TABLE_CREATION,
  all db.create_tables() calls, and commented-out SQLite scout_db code
- db_helpers.py: remove DATABASE_TYPE var and SQLite on_conflict_replace
  branch from upsert_many(); PostgreSQL ON CONFLICT is now the only path
- players.py: update stale comment
- tests/conftest.py: remove DATABASE_TYPE env var (no longer needed);
  keep POSTGRES_PASSWORD dummy for instantiation
- CLAUDE.md: update SQLite references to PostgreSQL

Note: unit tests in test_evolution_seed.py and test_season_stats_model.py
use SqliteDatabase(':memory:') for test isolation — this is legitimate test
infrastructure, not production SQLite compatibility code.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-23 04:45:54 +00:00
Claude reviewed 2026-03-23 04:47:27 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/db_engine.py (modified — ~280 lines removed)
  • app/db_helpers.py (modified)
  • app/routers_v2/players.py (modified — comment only)
  • tests/conftest.py (modified)
  • CLAUDE.md (modified)

Findings

Correctness

  • db_engine.py: Removal is complete and correct. All 13 SKIP_TABLE_CREATION guards removed, the SQLite else branch removed, and the commented-out scout_db block (~160 lines of dead commented code) cleaned up. PooledPostgresqlDatabase is now initialized unconditionally at module load, which is the correct outcome.
  • db_helpers.py: The DATABASE_TYPE == "postgresql" branch in upsert_many() was the only live SQLite code path. Removing the branch and keeping the PostgreSQL logic is correct. The removed imports (os, Union, SQL) are confirmed unused in the post-PR version — Union never appeared in any function signature, and SQL was imported but never referenced in a function body.
  • conftest.py: os.environ["DATABASE_TYPE"] = "postgresql" was the mechanism that previously prevented SQLite table-creation from running during test collection. With SKIP_TABLE_CREATION gone, this line is no longer needed. The POSTGRES_PASSWORD dummy credential is correctly kept — PooledPostgresqlDatabase still instantiates at import time and needs a non-None password.
  • Scope completeness: Verified via full-codebase grep — DATABASE_TYPE and SKIP_TABLE_CREATION appear only in the three files this PR modifies. No routers or other modules import DATABASE_TYPE from db_helpers. The removal is exhaustive.

Security

  • No issues. No credentials introduced or exposed. Token logging patterns unchanged.

Style & Conventions

  • The db_helpers.py module docstring at the top (the "Usage" block showing from app.db_helpers import upsert_many, DATABASE_TYPE) is updated in the diff to remove the DATABASE_TYPE reference — correct.
  • Blank line normalization after removed db.create_tables() blocks leaves some double-blank-lines in db_engine.py (e.g., after Rarity, Event, Cardset, MlbPlayer, Player, Team class definitions). This is a minor formatting inconsistency but is consistent with the existing style in that file and does not affect behavior.

Suggestions

  • POSTGRES_PORT is not listed in CLAUDE.md's Key Env Vars section, but it is used in db_engine.py (os.environ.get("POSTGRES_PORT", "5432")). This was already missing pre-PR, so it is not introduced by this change — but worth noting as a minor documentation gap if CLAUDE.md is ever revisited.

Verdict: COMMENT (APPROVED)

Clean, focused removal with no functional regressions. The scope is exactly what the PR title claims — all SQLite compatibility code in application modules is gone, the test scaffolding is updated to match, and the documentation is corrected. The DATABASE_TYPE removal is confirmed exhaustive across the codebase. No issues found that would block merging.

(Posted as COMMENT per Gitea self-review restriction.)

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified — ~280 lines removed) - `app/db_helpers.py` (modified) - `app/routers_v2/players.py` (modified — comment only) - `tests/conftest.py` (modified) - `CLAUDE.md` (modified) ### Findings #### Correctness - **db_engine.py**: Removal is complete and correct. All 13 `SKIP_TABLE_CREATION` guards removed, the SQLite `else` branch removed, and the commented-out `scout_db` block (~160 lines of dead commented code) cleaned up. `PooledPostgresqlDatabase` is now initialized unconditionally at module load, which is the correct outcome. - **db_helpers.py**: The `DATABASE_TYPE == "postgresql"` branch in `upsert_many()` was the only live SQLite code path. Removing the branch and keeping the PostgreSQL logic is correct. The removed imports (`os`, `Union`, `SQL`) are confirmed unused in the post-PR version — `Union` never appeared in any function signature, and `SQL` was imported but never referenced in a function body. - **conftest.py**: `os.environ["DATABASE_TYPE"] = "postgresql"` was the mechanism that previously prevented SQLite table-creation from running during test collection. With `SKIP_TABLE_CREATION` gone, this line is no longer needed. The `POSTGRES_PASSWORD` dummy credential is correctly kept — `PooledPostgresqlDatabase` still instantiates at import time and needs a non-None password. - **Scope completeness**: Verified via full-codebase grep — `DATABASE_TYPE` and `SKIP_TABLE_CREATION` appear only in the three files this PR modifies. No routers or other modules import `DATABASE_TYPE` from `db_helpers`. The removal is exhaustive. #### Security - No issues. No credentials introduced or exposed. Token logging patterns unchanged. #### Style & Conventions - The `db_helpers.py` module docstring at the top (the "Usage" block showing `from app.db_helpers import upsert_many, DATABASE_TYPE`) is updated in the diff to remove the `DATABASE_TYPE` reference — correct. - Blank line normalization after removed `db.create_tables()` blocks leaves some double-blank-lines in db_engine.py (e.g., after `Rarity`, `Event`, `Cardset`, `MlbPlayer`, `Player`, `Team` class definitions). This is a minor formatting inconsistency but is consistent with the existing style in that file and does not affect behavior. #### Suggestions - `POSTGRES_PORT` is not listed in CLAUDE.md's `Key Env Vars` section, but it is used in `db_engine.py` (`os.environ.get("POSTGRES_PORT", "5432")`). This was already missing pre-PR, so it is not introduced by this change — but worth noting as a minor documentation gap if CLAUDE.md is ever revisited. ### Verdict: COMMENT (APPROVED) Clean, focused removal with no functional regressions. The scope is exactly what the PR title claims — all SQLite compatibility code in application modules is gone, the test scaffolding is updated to match, and the documentation is corrected. The `DATABASE_TYPE` removal is confirmed exhaustive across the codebase. No issues found that would block merging. (Posted as COMMENT per Gitea self-review restriction.) --- *Automated review by Claude PR Reviewer*
cal requested changes 2026-03-23 13:14:43 +00:00
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/db_engine.py (modified — ~280 lines removed)
  • app/db_helpers.py (modified)
  • app/routers_v2/players.py (modified — comment update)
  • tests/conftest.py (modified)
  • CLAUDE.md (modified)

Findings

Correctness

The core logic changes are correct and complete. PooledPostgresqlDatabase is initialized unconditionally, all 13 db.create_tables() guards are removed, and the upsert_many() SQLite branch is removed. The PostgreSQL-only code path is functionally identical to the old DATABASE_TYPE == "postgresql" branch. No logic regressions found.

The conftest.py change is correct — the os.environ["DATABASE_TYPE"] = "postgresql" line is no longer needed and its removal is clean. The POSTGRES_PASSWORD dummy default is correctly retained.

One correctness concern: The Union import is removed from db_helpers.py, and the SQL import is also removed. Checking the post-PR file: SQL was imported from peewee but only used in the old SQLite branch (it was never used in the PostgreSQL path). Removing it is correct. However, the EXCLUDED import inside the loop (from peewee import EXCLUDED) is now unconditional and repeated on every batch iteration. This was already true for the PostgreSQL branch before — it is not a regression introduced by this PR — but it is worth noting as a pre-existing style issue.

Security

No security issues. No secrets introduced, no injection vectors changed, no access control logic touched.

Style & Conventions

Issue — Stale compose.production.yml: The compose.production.yml file still passes DATABASE_TYPE=postgresql as an environment variable to the container (line 19). After this PR merges, db_engine.py no longer reads that variable. The env var becomes dead configuration, which is harmless at runtime but misleading. This file is not in the PR diff. A follow-up cleanup is warranted.

Issue — Stale docs and .env.example: docs/QUICK_START.md, docs/MIGRATION_PROGRESS.md, docs/POSTGRES_MIGRATION_GUIDE.md, docs/DOCKER_QUICKSTART.md, .env.example, and docs/PROJECT_PLAN.json all still reference DATABASE_TYPE, the SQLite fallback, and export DATABASE_TYPE=sqlite. These are not updated in this PR. They will mislead future contributors or anyone running the dev setup. This should be addressed in a follow-up issue or expanded scope of this PR.

Issue — CLAUDE.md on-disk is stale: The CLAUDE.md in database/ on the main branch still reads "Peewee ORM with SQLite (WAL mode)" and lists DATABASE_TYPE in Key Env Vars. This PR correctly updates it. However, the file in the repo on main also references a stale Release Workflow section mentioning next-release staging branch, which the parent CLAUDE.md says is retired. This is pre-existing and out of scope.

Minor — db_helpers.py docstring: The PR correctly removes DATABASE_TYPE from the module docstring's usage example. No issues.

Edge Cases

No new edge cases introduced. The removal of db.create_tables() calls is safe — tables are managed by migrations in PostgreSQL, not auto-created at import time. This was already the effective behavior whenever DATABASE_TYPE=postgresql was set.

Test Coverage

The PR author notes that tests/test_evolution_seed.py and tests/test_season_stats_model.py intentionally use SqliteDatabase(':memory:') and are left in place — this is correct behavior. These tests do in-memory model binding and are not production compatibility code.

No new tests are added, which is acceptable for a pure removal/cleanup PR. The removal of DATABASE_TYPE from conftest.py is the only test-infrastructure change and it is correct.

Verdict: REQUEST_CHANGES

The application code changes in db_engine.py, db_helpers.py, players.py, and conftest.py are correct and complete. However, there is one blocking issue:

compose.production.yml still passes DATABASE_TYPE=postgresql to the container. While this is harmless at runtime (the variable is simply ignored post-merge), it creates misleading configuration in the production deployment file and should be removed as part of this cleanup PR to keep the change complete.

Additionally, the docs (docs/QUICK_START.md, docs/MIGRATION_PROGRESS.md, .env.example, etc.) still reference DATABASE_TYPE and SQLite. These are pervasive enough that they should be addressed — either in this PR or via a follow-up issue explicitly tracked. Please either expand the scope to include them or open a tracking issue before merging.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified — ~280 lines removed) - `app/db_helpers.py` (modified) - `app/routers_v2/players.py` (modified — comment update) - `tests/conftest.py` (modified) - `CLAUDE.md` (modified) --- ### Findings #### Correctness The core logic changes are correct and complete. `PooledPostgresqlDatabase` is initialized unconditionally, all 13 `db.create_tables()` guards are removed, and the `upsert_many()` SQLite branch is removed. The PostgreSQL-only code path is functionally identical to the old `DATABASE_TYPE == "postgresql"` branch. No logic regressions found. The `conftest.py` change is correct — the `os.environ["DATABASE_TYPE"] = "postgresql"` line is no longer needed and its removal is clean. The `POSTGRES_PASSWORD` dummy default is correctly retained. **One correctness concern:** The `Union` import is removed from `db_helpers.py`, and the `SQL` import is also removed. Checking the post-PR file: `SQL` was imported from `peewee` but only used in the old SQLite branch (it was never used in the PostgreSQL path). Removing it is correct. However, the `EXCLUDED` import inside the loop (`from peewee import EXCLUDED`) is now unconditional and repeated on every batch iteration. This was already true for the PostgreSQL branch before — it is not a regression introduced by this PR — but it is worth noting as a pre-existing style issue. #### Security No security issues. No secrets introduced, no injection vectors changed, no access control logic touched. #### Style & Conventions **Issue — Stale `compose.production.yml`:** The `compose.production.yml` file still passes `DATABASE_TYPE=postgresql` as an environment variable to the container (line 19). After this PR merges, `db_engine.py` no longer reads that variable. The env var becomes dead configuration, which is harmless at runtime but misleading. This file is not in the PR diff. A follow-up cleanup is warranted. **Issue — Stale docs and `.env.example`:** `docs/QUICK_START.md`, `docs/MIGRATION_PROGRESS.md`, `docs/POSTGRES_MIGRATION_GUIDE.md`, `docs/DOCKER_QUICKSTART.md`, `.env.example`, and `docs/PROJECT_PLAN.json` all still reference `DATABASE_TYPE`, the SQLite fallback, and `export DATABASE_TYPE=sqlite`. These are not updated in this PR. They will mislead future contributors or anyone running the dev setup. This should be addressed in a follow-up issue or expanded scope of this PR. **Issue — CLAUDE.md on-disk is stale:** The CLAUDE.md in `database/` on the `main` branch still reads "Peewee ORM with SQLite (WAL mode)" and lists `DATABASE_TYPE` in Key Env Vars. This PR correctly updates it. However, the file in the repo on `main` also references a stale `Release Workflow` section mentioning `next-release` staging branch, which the parent `CLAUDE.md` says is retired. This is pre-existing and out of scope. **Minor — `db_helpers.py` docstring:** The PR correctly removes `DATABASE_TYPE` from the module docstring's usage example. No issues. #### Edge Cases No new edge cases introduced. The removal of `db.create_tables()` calls is safe — tables are managed by migrations in PostgreSQL, not auto-created at import time. This was already the effective behavior whenever `DATABASE_TYPE=postgresql` was set. #### Test Coverage The PR author notes that `tests/test_evolution_seed.py` and `tests/test_season_stats_model.py` intentionally use `SqliteDatabase(':memory:')` and are left in place — this is correct behavior. These tests do in-memory model binding and are not production compatibility code. No new tests are added, which is acceptable for a pure removal/cleanup PR. The removal of `DATABASE_TYPE` from `conftest.py` is the only test-infrastructure change and it is correct. --- ### Verdict: REQUEST_CHANGES The application code changes in `db_engine.py`, `db_helpers.py`, `players.py`, and `conftest.py` are correct and complete. However, there is one blocking issue: **`compose.production.yml` still passes `DATABASE_TYPE=postgresql` to the container.** While this is harmless at runtime (the variable is simply ignored post-merge), it creates misleading configuration in the production deployment file and should be removed as part of this cleanup PR to keep the change complete. Additionally, the docs (`docs/QUICK_START.md`, `docs/MIGRATION_PROGRESS.md`, `.env.example`, etc.) still reference `DATABASE_TYPE` and SQLite. These are pervasive enough that they should be addressed — either in this PR or via a follow-up issue explicitly tracked. Please either expand the scope to include them or open a tracking issue before merging. --- *Automated review by Claude PR Reviewer*
All checks were successful
Build Docker Image / build (pull_request) Successful in 8m32s
Details
This pull request has changes conflicting with the target branch.
  • CLAUDE.md
  • app/db_engine.py
  • tests/conftest.py
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin ai/paper-dynasty-database#122:ai/paper-dynasty-database#122
git checkout ai/paper-dynasty-database#122
Sign in to join this conversation.
Reviewers
No reviewers
cal
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
2 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#126
No description provided.
Delete Branch "ai/paper-dynasty-database#122"

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?